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

support for Add Filter button #3671

Merged
merged 23 commits into from
Dec 18, 2023
Merged

support for Add Filter button #3671

merged 23 commits into from
Dec 18, 2023

Conversation

briangregoryholmes
Copy link
Contributor

Adds an "Add Filter" button to the Filters component. Also updates relevant menu designs.

Closes #3620

@briangregoryholmes briangregoryholmes self-assigned this Dec 8, 2023
Copy link
Collaborator

@AdityaHegde AdityaHegde left a comment

Choose a reason for hiding this comment

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

A few UX questions,

  1. Should a check mark be shown and the dimension be disabled in add filter if it is already present in the filters?
  2. Or should the dimension filter dropdown open if an existing dimension is selected in add filter drop down?

@@ -73,7 +73,6 @@ export const getFilterSearchList = (
measureNames: [metricsExplorer.leaderboardMeasureName],
timeStart: timeControls.timeStart,
timeEnd: timeControls.timeEnd,
limit: "15",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to remove this? Better to have an explicit limit here (backend overrides limit to 100).

Copy link
Contributor Author

@briangregoryholmes briangregoryholmes Dec 15, 2023

Choose a reason for hiding this comment

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

This was removed because of the design changes to the dropdown. Previously, the dropdown only displayed the already selected values plus a search bar that would bring up more. The new dropdown ostensibly shows the full list of available options and even provides an opportunity to select or deselect all. Totally agree that it would be nice if we could put a cap on it somehow, but I'm not sure I see a way given the design requirements and my current understanding of the runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm would probably be better to have an explicit limit of 100. Otherwise a frontend engineer will need to look at go code to know the limit.

@@ -33,7 +33,7 @@ export function toggleDimensionValueSelection(
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a safeguard for no dimensionValue in the above if block. Right now an undefined gets pushed. You can see this by adding a dimension which is already present in the filters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually a separate method to just an entry in the filters without a value might be better than reusing this method. Keeps things simple. The FilterButton will call that new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a toggleDimensionNameSelection method

web-local/test/ui/dashboards.spec.ts Outdated Show resolved Hide resolved
@briangregoryholmes
Copy link
Contributor Author

New commits up now. Regarding your UX question, I think the best option is to simply remove the already selected filters from that dropdown. The existing filters wrap onto a new line (and so are always visible) and the button is specifically for adding a filter (rather than removing or navigating to), so I think it's better UX to only present valid options. Good callout. Let me know what you think.

Copy link
Collaborator

@AdityaHegde AdityaHegde left a comment

Choose a reason for hiding this comment

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

Just a couple of small nits. Looks good otherwise.

@AdityaHegde AdityaHegde merged commit cec766e into main Dec 18, 2023
2 checks passed
@AdityaHegde AdityaHegde deleted the filter branch December 18, 2023 06:20
mindspank pushed a commit that referenced this pull request Dec 18, 2023
* rework of footer to match new design and improve readability

* adding filter button functionality, plus design tweaks to relevant menus

* new icon

* chip should not be removed when deselecting the only selected value

* removing limit from filter list query

* changed searchedValues to allValues, simplified display logic

* updated tests to meet updated requirements

* prettier unused vars build fix

* remove keep-alive event

* update let directive for named slot, add timeout to dashboard.spec

* remove unused function

* change to active logic when mounting chip component

* resolve name collision

* update page wait from timeout to selector

* added specific method for adding a dimension name without a value

* only unselected dimension names are shown in the add filter dropdown

* add back limit

* toggleDimensionNameSelection is now actually a toggling function

* dispatch remove event rather than handle remove directly, plus bind to active property

* use state manager action and prop cleanup

* add back limit
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.

Support for Add Filter button
2 participants