Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance batch action parameters #1321

Merged
merged 9 commits into from
Jul 12, 2022

Conversation

mooror
Copy link
Contributor

@mooror mooror commented Jul 6, 2022

Description

This is the second of two PRs (the first was made in the cms module) for implementing the enhancement described in the "Batch actions form - Custom parameter fields enhancement" issue

Changes

  • Added the parameter fields created by CMSMain's BatchActionParameters() method to the BatchActionsForm
  • Added javascript for displaying/hiding parameter fields when the batch action dropdown is changed
  • Added CSS styles to make the parameter fields render properly under the batch action dropdown

mooror and others added 4 commits July 5, 2022 17:17
+ Added the parameter fields created by CMSMain's BatchActionParameters() method to the BatchActionsForm
+ Added javascript for displaying/hiding parameter fields when the batch action dropdown is changed
+ Added CSS styles to make the parameter fields render properly under the batch action dropdown
@mooror
Copy link
Contributor Author

mooror commented Jul 6, 2022

One thing I noted was that each of the commits was labeled with a different author. I was having trouble running builds on windows, so I used another workstation which was configured with a different author so 🤷‍♂️ hopefully that won't be an issue

But please let me know if I should fix that (and how)

@GuySartorelli
Copy link
Member

each of the commits was labeled with a different author. I was having trouble running builds on windows, so I used another workstation which was configured with a different author so man_shrugging hopefully that won't be an issue

It's not a problem. The commits will be squashed anyway before merging. If you want to control the name used in the changelog, you might want to squash the commits yourself so that you can control who the commit is authored by.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few changes mostly from a code quality POV

client/src/legacy/LeftAndMain.BatchActions.js Outdated Show resolved Hide resolved
client/src/legacy/LeftAndMain.BatchActions.js Outdated Show resolved Hide resolved
client/src/legacy/LeftAndMain.BatchActions.js Outdated Show resolved Hide resolved
client/src/legacy/LeftAndMain.BatchActions.js Outdated Show resolved Hide resolved
client/src/styles/legacy/_style.scss Outdated Show resolved Hide resolved
code/LeftAndMain.php Outdated Show resolved Hide resolved
+ Added code to loop over parameter input fields and reset their values to the default before displaying them
@mooror
Copy link
Contributor Author

mooror commented Jul 7, 2022

Just as a heads up, commit 68abd1a adds extra functionality for resetting parameter fields before they are displayed. This fixes an issue I found that was causing field values to be kept even after the "Go" button was pressed and the action successfully completed (which is not what we want).

I placed the reset functionality in the dropdown onchange event (instead of the "go" button click event) so that it would also clear values when the user changed actions (via the action dropdown). My thinking is that batch actions make large changes across many records, and we want to reduce the chances for user error.

mooror and others added 3 commits July 6, 2022 22:51
+ Replaced the ID `#BatchActionParameters` selector with a `.action-parameters` class selector
+ Nested `.action-parameters` rules under `.cms-batch-actions`
+ Removed the changes to the BatchActionsForm in favor of using `updateBatchActionsForm()` in an extension
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well locally. Without the accompanying PR in cms this causes no problems. With the accompanying PR this works great.
Thank you for this contribution!

@GuySartorelli GuySartorelli merged commit 7cf2435 into silverstripe:1 Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants