-
Notifications
You must be signed in to change notification settings - Fork 1
LPD-70790 Implement FDS search and filters management through global state v2 #5213
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
base: master
Are you sure you want to change the base?
Conversation
|
CI is automatically triggering the following test suites:
|
|
Test suite sf has been triggered on http://test-1-37 |
Summary of ChangesHello @markocikos, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Frontend Data Set (FDS) component by integrating a global state mechanism for managing its search and filter functionalities. This change enables seamless interaction between the FDS and other parts of the page, such as custom fragments, allowing for more dynamic and interconnected user interfaces. The inclusion of sample fragments provides clear examples of how to leverage this new capability, while underlying utility functions and dependencies have been updated to support the new architecture. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request successfully implements search and filter management through a global state, which is a great improvement for modularity and interactivity. The addition of fragment samples effectively demonstrates the new capability. The refactoring of isReducedMotion and the introduction of deepClone are also valuable changes. I've found a few areas for improvement, mainly related to state management patterns and a potential bug in the deepClone utility. My comments provide suggestions to enhance robustness and simplify the code.
...ps/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/main/utils/deepClone.ts
Outdated
Show resolved
Hide resolved
...end-data-set/frontend-data-set-web/src/main/resources/META-INF/resources/FrontendDataSet.tsx
Show resolved
Hide resolved
...end-data-set/frontend-data-set-web/src/main/resources/META-INF/resources/FrontendDataSet.tsx
Show resolved
Hide resolved
.../frontend-data-set-sample-web/src/main/resources/META-INF/resources/js/fragments/Filters.tsx
Show resolved
Hide resolved
| Record<KEYS, AccessibilityMenuSetting> | ||
| >('accessibility-menu', {} as any); | ||
|
|
||
| export {isReducedMotion} from './isReducedMotion'; |
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.
Moving isReducedMotion to be able to place deepClone in frontend-js-web. Without the move, there would be a circular dependency between frontend-js-web and frontend-js-state-web, as frontend-js-state-web needs to use deepClone.
✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutesRan com.liferay.source.formatter at released version 1.0.1554. Click here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-70790-2 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#14245 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#5213 Testray Routine:EE Pull Request Testray Build ID: 381756987Testray Importer:test-portal-source-format#14245 |
| const filtersGroups = [ | ||
| {filters: ['date', 'color'], label: 'Group 1'}, | ||
| {filters: ['invalid', 'size'], label: 'Group 2'}, | ||
| {filters: ['clientExtension', 'invalid', 'size'], label: 'Group 2'}, |
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.
This is a fix of unrelated bug in master, test failures introduced with #5199. CX was not included in advanced sample.
This unfortunately means that our full CI tests are unreliable 😞
|
|
||
| await page.getByRole('button', {name: 'Show Results'}).click(); | ||
|
|
||
| await waitForFDS({page}); |
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.
A bunch of flakiness fixes.
| </div> | ||
|
|
||
| <ActiveFiltersBar | ||
| <ResultsBar |
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.
Bar contains not only actions, but also search results.
| disabled={disabled} | ||
| displayType="unstyled" | ||
| onClick={resetFiltersValue} | ||
| onClick={onClearResultsBar} |
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.
Fixing separation of concerns, like here, was necessary in several places.
| | IDateFilterState | ||
| | ISelectionFilterState; | ||
|
|
||
| interface IFDSState { |
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.
All new interfaces have State keyword. The idea is to separate component interfaces and state value (prop) interfaces, by a convention. We often mix those in code, leading to code that is hard to read, and TS gymnastics.
| * @param sourceObj The object to clone. | ||
| * @returns A deep clone of the object. | ||
| */ | ||
| export default function deepClone(object: any) { |
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.
If needed, we can expand this with prevention of circular references. I don't really see the case when it is needed.
| if ( | ||
| globalFDSStateInitialized || | ||
| !filterClientExtensionsLoaded || | ||
| !cellClientExtensionsLoaded | ||
| ) { | ||
| return; | ||
| } |
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.
Don't push initial state updates until CXs are loaded.
|
ci:test:relevant |
|
Test suite relevant has been triggered on http://test-1-38 |
|
ci:test:frontend-data-set |
|
Test suite frontend-data-set has been triggered on http://test-1-35 |
|
ci:test:sf |
|
Test suite sf has been triggered on http://test-1-33 |
… prevents future bugs and keeps TS clean. Original global state value is still key as dependency for `useEffect`s.
❌ ci:test:stable - 6 out of 12 jobs passed❌ ci:test:relevant - 10 out of 21 jobs passed in 1 hour 44 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: f107373318f0a3024a15a3849c857a4d53259545 ci:test:stable - 6 out of 12 jobs PASSED6 Failed Jobs:
6 Successful Jobs:ci:test:relevant - 10 out of 21 jobs PASSED11 Failed Jobs:
10 Successful Jobs:For more details click here.Failures unique to this pull:
BUILD SUCCESSFUL For upstream results, click here.Test bundle downloads: |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#24874 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#5213 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - markocikos > liferay-frontend - PR#5213 - 2025-12-04[23:20:20] Testray Build ID: 383689076Testray Importer:test-portal-acceptance-pullrequest(master)#24874 |
|
ci:test:stable |
|
Test suite stable has been triggered on http://test-1-37 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#24877 Jenkins Report:jenkins-report.html Jenkins Suite:stable Pull Request:liferay-frontend#5213 Testray Routine:EE Pull Request Testray Build:[master] ci:test:stable - markocikos > liferay-frontend - PR#5213 - 2025-12-05[02:00:15] Testray Build ID: 383769586Testray Importer:test-portal-acceptance-pullrequest(master)#24877 |
b3326ba to
ea2a0b5
Compare
|
ci:test:relevant |
|
Test suite relevant has been triggered on http://test-1-39 |
❌ ci:test:stable - 7 out of 12 jobs passed❌ ci:test:relevant - 12 out of 22 jobs passed in 2 hours 4 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: feff86712bf0eeba59f68873f5bd0eb989a50337 ci:test:stable - 7 out of 12 jobs PASSED5 Failed Jobs:
7 Successful Jobs:ci:test:relevant - 12 out of 22 jobs PASSED10 Failed Jobs:
12 Successful 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)#15365 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#5213 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - markocikos > liferay-frontend - PR#5213 - 2025-12-05[05:58:28] Testray Build ID: 383899289Testray Importer:test-portal-acceptance-pullrequest(master)#15365 |
References
What is the goal of this PR?
With this PR, FDS manages search and filters though global state. This enables JS code from any other parts of the page to interact with it. In PR, we are adding fragment samples that showcase this wiring.
See technical notes inline.
What does it look like?
vokoscreenNG-2025-12-03_12-42-09.mp4