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

feat: Apply filters follow up #6034

Merged
merged 16 commits into from
Mar 11, 2025
Merged

feat: Apply filters follow up #6034

merged 16 commits into from
Mar 11, 2025

Conversation

ondrejvelisek
Copy link
Contributor

@ondrejvelisek ondrejvelisek commented Mar 10, 2025

fix(sdk-ui-dashboard): make working state update in attribute filter

feat(sdk-ui-dashboard): show working selection in filter button

  • when apply all at once config is used
  • fix inverted logic for attribute working selection

fix(sdk-ui-dashboard): make working state update date filter

feat(sdk-ui-dashboard): remove apply from attribute filter

  • and rename cancel to close

feat(sdk-ui-dashboard): remove apply from date filter

  • and rename cancel to close

feat(sdk-ui-dashboard): remove attribute filter status bar

feat(sdk-ui-dashboard): add unapplied filters banner

  • hide apply button instead of diable it when no changes
  • fix reversed selector

feat(sdk-ui-dashboard): make reset btn reset working filters too

  • add reset all working filters command
  • fix depcruiser errors

fix(sdk-ui): missing localization entry for close button

feat(sdk-ui-dashboard): support saved views in apply all at once

fix(sdk-ui-dashboard): responsivness in apply all at once mode

feat(sdk-ui-dashboard): make crossfiltering work properly

  • trigger filter context change event for workign state changes too
  • handle filter invalid state properly

feat(sdk-ui-dashboard): hide new apply all filters behind FF

  • enableDashboardFiltersApplyModes

Important

Please, don't forget to run rush change for the commits that introduce new features or significant changes 🙏 This information is used to generate the change log.


Run extended test by pull request comment

Commands can be triggered by posting a comment with specific text on the pull request. It is possible to trigger multiple commands simultaneously.

extended-test --backstop | --integrated | --isolated | --record [--filter <file1>,<file2>,...,<fileN>]

Explanation

  • --backstop The command to run screen tests.
  • --integrated The command to run integrated tests against the live backend.
  • --isolated The command to run isolated tests against recordings.
  • --record The command to create new recordings for isolated tests.
  • --filter (Optional) A comma-separated list of test files to run. This parameter is valid only for the --integrated, --isolated, and --record commands.

Examples

extended-test --backstop
extended-test --integrated
extended-test --integrated --filter test1.spec.ts,test2.spec.ts
extended-test --isolated
extended-test --isolated --filter test1.spec.ts,test2.spec.ts
extended-test --record
extended-test --record --filter test1.spec.ts,test2.spec.ts

- when apply all at once config is used
- fix inverted logic for attribute working selection

JIRA: GDP-2938
risk: low
- and rename cancel to close

JIRA: GDP-2938
risk: low
- and rename cancel to close

JIRA: GDP-2938
risk: low
- hide apply button instead of diable it when no changes
- fix reversed selector

JIRA: GDP-2938
risk: low
- add reset all working filters command
- fix depcruiser errors

JIRA: GDP-2938
risk: low
- trigger filter context change event for workign state changes too
- handle filter invalid state properly

JIRA: GDP-2938
risk: low
- enableDashboardFiltersApplyModes

JIRA: GDP-2938
risk: low
JIRA: GDP-2938
risk: low
@kandl
Copy link
Contributor

kandl commented Mar 11, 2025

extended-test --backstop

Copy link

"extended-test --backstop" started. Check the progress here.

"changes": [
{
"packageName": "@gooddata/sdk-ui-all",
"comment": "Support of new filter mode Apply all at once",
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be clear that it's a Dashboard component feature

@@ -84,6 +96,7 @@ export function DraggableAttributeFilter({
>
<FilterComponent
filter={filterToUse}
workingFilter={enableDashboardFiltersApplyModes ? workingFilterToUse : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this prop really required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion on Slack we decided to deprecate this prop for now and remove it in the future phase 5 refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will pass workingFilterToUse into regular filter prop instead.

Which means the inner filter state will think those incoming working filters are applied. But we do not need to distinguish between applied and working state for now. And we prefer to keep filters component API simple.

Similar change should be done for DateFilter.

Copy link

❌ "extended-test --backstop" finished with result failure. Check the results here.

- we will remove them and use regular variants for sync working state instead
- improve changelog message

JIRA: GDP-2938
risk: low
@ondrejvelisek ondrejvelisek force-pushed the ovel/apply-filters-phase-4 branch from e6fbd76 to 87a3d19 Compare March 11, 2025 09:00
- not sure why TypeScript check is not catching this

JIRA: GDP-2938
risk: low
@ondrejvelisek
Copy link
Contributor Author

extended-test --backstop

Copy link

"extended-test --backstop" started. Check the progress here.

Copy link

❌ "extended-test --backstop" finished with result failure. Check the results here.

@kandl
Copy link
Contributor

kandl commented Mar 11, 2025

extended-test --integrated

Copy link

"extended-test --integrated" started. Check the progress here.

Copy link

✅ "extended-test --integrated" finished with result success. Check the results here.

Copy link
Contributor

@kandl kandl left a comment

Choose a reason for hiding this comment

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

LGTM

@kandl kandl enabled auto-merge March 11, 2025 10:03
@kandl kandl merged commit d552e94 into master Mar 11, 2025
15 checks passed
@kandl kandl deleted the ovel/apply-filters-phase-4 branch March 11, 2025 10:13
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.

2 participants