-
Notifications
You must be signed in to change notification settings - Fork 121
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
Ovel/apply filters logic #5985
base: master
Are you sure you want to change the base?
Ovel/apply filters logic #5985
Conversation
- the callback is called when filter working selection is changed risk: low JIRA: GDP-2938
- update the state on attribute filters change risk: low JIRA: GDP-2938
- add command to apply staged working filters risk: low JIRA: GDP-2938
- dispatch setAttributeFilterDisplayForm cmd into working state - dispatch FilterContextChanged event after apply all JIRA: GDP-2938 risk: low
- fix wrong working negative selection JIRA: GDP-2938 risk: low
JIRA: GDP-2938 risk: low
- only when filters apply mode all_at_once JIRA: GDP-2938 risk: low
…lters - only when filters apply mode all_at_once JIRA: GDP-2938 risk: low
JIRA: GDP-2938 risk: low
- this way we do not need to synchronize other fields - which makes it easier to maintain.his JIRA: GDP-2938 risk: low
*/ | ||
export function applyFilterContext( | ||
filterContext: IFilterContextDefinition, | ||
workingFilterContext: IWorkingFilterContextDefinition | undefined, |
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.
Why is new IWorkingFilterContextDefinition
type required, why not use IFilterContextDefinition
for both definitions? I'm also thinking whether it belongs to sdk-model
, as this is strictly dashboard runtime implementation detail.
@@ -706,12 +710,13 @@ export interface ChangeAttributeFilterSelection extends IDashboardCommand { | |||
} | |||
|
|||
// @public | |||
export function changeAttributeFilterSelection(filterLocalId: string, elements: IAttributeElements, selectionType: AttributeFilterSelectionType, correlationId?: string): ChangeAttributeFilterSelection; | |||
export function changeAttributeFilterSelection(filterLocalId: string, elements: IAttributeElements, selectionType: AttributeFilterSelectionType, correlationId?: string, isWorkingSelectionChange?: boolean): ChangeAttributeFilterSelection; |
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.
What about adding new changeWorkingAttributeFilterSelection
function? (having isWorkingSelectionChange
after correlationId
is not very nice). It can dispatch exactly the same action, just with isWorkingSelectionChange flag set to true.
* | ||
* @alpha | ||
*/ | ||
export const selectAreAllFiltersApplied: DashboardSelector<boolean | undefined> = createSelector( |
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 would rather name it selectIsWorkingFilterContextChanged
or something like that
@@ -15,6 +15,9 @@ import { | |||
IDashboardAttributeFilter, | |||
IDashboardDateFilter, | |||
ObjRef, | |||
type IAttributeElement, |
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.
Nitpick: use type
in type imports is not required - it's required only when re-exporting types in barel or main index files. (because esbuild and other tools have issue with removing types from re-exports, but it's not issue anywhere else).
@@ -136,6 +137,7 @@ export const DefaultCommandHandlers: { | |||
"GDC.DASH/CMD.FILTER_CONTEXT.DATE_FILTER.ADD": addDateFilterHandler, | |||
"GDC.DASH/CMD.FILTER_CONTEXT.DATE_FILTER.REMOVE": removeDateFiltersHandler, | |||
"GDC.DASH/CMD.FILTER_CONTEXT.DATE_FILTER.MOVE": moveDateFilterHandler, | |||
"GDC.DASH/CMD.FILTER_CONTEXT.APPLY_ALL": applyAllFilterContextHandler, |
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.
Should it be called APPLY_SELECTION
or APPLY_WORKING_SELECTION
(so it's aligned with CHANGE_SELECTION
)?
* | ||
* @alpha | ||
*/ | ||
workingFilterContextDefinition?: IWorkingFilterContextDefinition; |
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.
Here should be mention about filter mode setting.
feat(sdk-ui-filters): add onSelect callback prop to attribute filter
feat(sdk-ui-filters): create new state for working filters
feat(sdk-ui-dashboard): add apply button to dashboard filter bar
fix(sdk-ui-dashboard): reloading insights after filter select
feat(sdk-ui-dashboard): make apply all button disable if no change
feat(sdk-ui-dashboard): make date filter change working filter state
feat(sdk-ui-dashboard): make child filters use working parent filters
feat(sdk-ui-dashboard): make child filters use working parent date filters
feat(sdk-ui-dashboard): translate filters all at once apply button
feat(sdk-ui-dashboard): store only changes in working filter state
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.
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