-
Notifications
You must be signed in to change notification settings - Fork 1
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
LPD-42578 Add "Clear" Action to Remove Selection #4763
base: master
Are you sure you want to change the base?
Conversation
CI is automatically triggering the following test suites:
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-42578 1 Successful Jobs:For more details click here. |
Jenkins Build:test-portal-source-format#7592 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4763 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - chestofwonders > liferay-frontend - PR#4763 - 2025-02-26[00:15:55] Testray Build ID: Testray Importer:publish-testray-report#27535 |
href="#" | ||
onClick={(event) => { | ||
event.preventDefault(); | ||
deselectItems(selectedItemsValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if we can pass a null
value or a string (all
) to this function so we can set the state as and empty array instead of iterating over all items in the FrontendDataSet#deselectItems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally more on the side of keeping the API of the methods simple... how about asking a question at the beginning of the implementation of deselectItems
? Asking if selectedItemsValues
has the same length as the parameter sent, should work... 😔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on having a simple API. What you suggest is fine 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a question inline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chestofwonders please see comment inline
@@ -43,6 +43,7 @@ function ManagementBar({ | |||
{selectionType === 'multiple' && ( | |||
<BulkActions | |||
bulkActions={bulkActions} | |||
deselectItems={deselectItems} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is technically correct, it's a bad pattern. You never want to push callbacks down to child components. The reason is it violates separation of concerns; with this your child component contains logic that is implemented on the parent level.
Instead, you want to have a generic callback in the child component, that makes sense for that component in isolation. Something like onClear
or onClearClick
or onClearButtonClick
prop in BulkActions, that is propagated through onBulkActionsClear
prop in ManagementBar
, and ultimately implemented where the state is, in FrontendDataSet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored it. Probably there's more stuff around the whole FrontendDataSet
structure we want to address... maybe make an more extensive use of the Context, but for now should be enough, I think
15bafe8
to
4ed4e29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chestofwonders looks good, just a small nit in comment
...tend-data-set/frontend-data-set-web/src/main/resources/META-INF/resources/FrontendDataSet.js
Outdated
Show resolved
Hide resolved
4ed4e29
to
eda8aed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chestofwonders LGTM! ✨
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
❌ ci:test:relevant - 0 out of 1 jobs passed in 7 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 4281f9b41f4c84911eaaf42439d4bf4905772f58 ci:test:relevant - 0 out of 1 jobs PASSED1 Failed Jobs:For more details click here.Failures unique to this pull:
For upstream results, click here.Test bundle downloads: |
❌ ci:test:sf - 0 out of 1 jobs passed in 10 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-42578 1 Failed Jobs:For more details click here.[exec] > Task :packageRunCheckFormat [exec] yarn run v1.13.0 [exec] \$ node-scripts check:ci [exec] [exec] ⚙️ Running preflight checks... [exec] [exec] ⚙️ Checking outdated tsconfig.json files ... [exec] [exec] ⚙️ Running TypeScript checks on modified files... [exec] ℹ️ A total of 12 CPUs were detected: launching tsc using 12 workers [exec] [exec] ⚙️ Running format checks on modified files... [exec] /opt/dev/projects/github/liferay-portal/modules/apps/frontend-data-set/frontend-data-set-web/src/main/resources/META-INF/resources/management_bar/ManagementBar.js [exec] 35:11 error 'deselectItems' is not defined. no-undef [exec] [exec] ✖ 1 problem (1 error, 0 warnings) [exec] [exec] [exec] ❌ CI checks failed. [exec] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. [exec] error Command failed with exit code 1. [exec] [exec] > Task :packageRunCheckFormat FAILED [exec] [exec] [exec] FAILURE: Build failed with an exception. [exec] [exec] * What went wrong: [exec] Execution failed for task ':packageRunCheckFormat'. [exec] > Process 'command '/opt/dev/projects/github/liferay-portal/build/node/bin/node'' finished with non-zero exit value 1 [exec] [exec] * Try: [exec] > Run with --info or --debug option to get more log output. [exec] > Run with --scan to get full insights. [exec] > Get more help at https://help.gradle.org. [exec] [exec] * Exception is: [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':packageRunCheckFormat'. [exec] at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda\$executeIfValid\$1(ExecuteActionsTaskExecuter.java:148) [exec] at org.gradle.internal.Try\$Failure.ifSuccessfulOrElse(Try.java:282) [exec] at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeIfValid(ExecuteActionsTaskExecuter.java:146) [exec] at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:134) [exec] at org.gradle.api.internal.tasks.execution.FinalizePropertiesTaskExecuter.execute(FinalizePropertiesTaskExecuter.java:46) [exec] at org.gradle.api.internal.tasks.execution.ResolveTaskExecutionModeExecuter.execute(ResolveTaskExecutionModeExecuter.java:51) |
Jenkins Build:test-portal-acceptance-pullrequest(master)#14608 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4763 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - chestofwonders > liferay-frontend - PR#4763 - 2025-03-04[08:30:20] Testray Build ID: Testray Importer:publish-testray-report#33872 |
Jenkins Build:test-portal-source-format#4158 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4763 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - chestofwonders > liferay-frontend - PR#4763 - 2025-03-04[08:30:22] Testray Build ID: Testray Importer:publish-testray-report#16412 |
eda8aed
to
cc10d62
Compare
ci:test:sf |
✔️ ci:test:sf - 1 out of 1 jobs passed in 10 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-42578 1 Successful Jobs:For more details click here. |
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
Skipping previously passed test suites:
|
❌ ci:test:relevant - 0 out of 1 jobs passed in 7 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 4281f9b41f4c84911eaaf42439d4bf4905772f58 ci:test:relevant - 0 out of 1 jobs PASSED1 Failed Jobs:For more details click here.Failures unique to this pull:
For upstream results, click here.Test bundle downloads: |
Jenkins Build:test-portal-acceptance-pullrequest(master)#12755 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4763 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - chestofwonders > liferay-frontend - PR#4763 - 2025-03-04[10:02:02] Testray Build ID: Testray Importer:publish-testray-report#27872 |
Jenkins Build:test-portal-source-format#7630 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4763 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - chestofwonders > liferay-frontend - PR#4763 - 2025-03-04[09:34:42] Testray Build ID: Testray Importer:publish-testray-report#28978 |
ci:test:relevant |
Jenkins Build:test-portal-acceptance-pullrequest(master)#11155 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4763 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - chestofwonders > liferay-frontend - PR#4763 - 2025-03-05[00:06:09] Testray Build ID: Testray Importer:publish-testray-report#26282 |
Issue: https://liferay.atlassian.net/browse/LPD-42578
Add "Clear" Action to Remove Selection
The goal of this PR is to provide a button to clear all selected items in data sets
Acceptance Criteria