-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add filter period to iframe src for custom widgets #3183
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for opening this Edson! 🙏 The logic looks good 👍
There's some things we should do to clean up the PR before we ask analytics folks to review it:
- Add a more descriptive PR name, using conventional commit style, like "feat: add filter period to iframe src for custom widgets"
- In the description, some code block styling is wrapping the screenshots, so the images aren't visible -- move those screenshots out of the code block so they're easy to see
- There are some erroneous changes in
en.pot
andFilterSelector.js
-- remove those - I added some inline comments in the code -- if you use the GitHub interface to apply them, make sure to use commit messages that follow the conventional commit style (start with
chore:
/fix:
/refactor:
)
Thanks again for setting this up! Let me know if you have any questions 🙂
Changed the delimiter for `pe` values from semicolon (`;`) to comma (`,`), aligning with the standard format for query parameters. This change improves consistency and simplifies downstream processing. Co-authored-by: Kai Vandivier <[email protected]>
Replaced the explicit `if` condition `itemFilters[FILTER_PE] && itemFilters[FILTER_PE].length` with the optional chaining expression `itemFilters[FILTER_PE]?.length`, as recommended by SonarQube. Co-authored-by: Kai Vandivier <[email protected]>
Updated the query parameter `pe` to `period` in the iframe source URL to improve readability and better match the naming convention of the org unit parameter. Co-authored-by: Kai Vandivier <[email protected]>
Deleted the `i18n/en.pot` file to prevent potential conflicts with other translations or resources. This resolves issues with unnecessary file changes during development and ensures consistency in localization files.
Cleaned up the `FilterSelector.js` file by removing redundant or unnecessary changes.
Reverted unintended changes in the `i18n/en.pot`
…ashboard-app into iframesrc-add-pe
Reverted unintended changes in the `FilterSelector.js` file
Thank you for submitting this pull request. Could you please have a look at the unit tests in file
|
…ameSrc - Added tests to validate the behavior of the period (pe) filter with single and multiple values. - Ensured proper handling when both pe and ou filters are applied together. - Included a test to confirm pe filter is omitted when empty. - Verified the generated iframeSrc matches the expected URL for all scenarios.
Hi Edson, |
…ameSrc - Added tests to validate the behavior of the period (pe) filter with single and multiple values. - Ensured proper handling when both pe and ou filters are applied together. - Included a test to confirm pe filter is omitted when empty. - Verified the generated iframeSrc matches the expected URL for all scenarios.
…ashboard-app into iframesrc-add-pe
Quality Gate passedIssues Measures |
Key features
pe
(period) as a query parameter in the iframe source URL.pe
parameter to the iframe source when the filter is applied.Description
This update ensures that the
pe
(period) filter is included in the iframe URL for dashboard items when the filter is applied. If thepe
filter is present initemFilters
, the periods are concatenated using,
as the delimiter and added as aperiod
query parameter in the iframe source.TODO
TESTS
Known issues
Screenshots
Example:
If the
itemFilters
contains: