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(frontend): None selection option in filters dropdown #1692

Closed
wants to merge 3 commits into from

Conversation

vunder
Copy link
Contributor

@vunder vunder commented Jan 4, 2025

Description

Adds "None" button to multi-selector within the filter's dropdown. Deselects all values

Related issue

Closes #1691

Copy link

vercel bot commented Jan 4, 2025

@vunder is attempting to deploy a commit to the measure Team on Vercel.

A member of the Team first needs to authorize it.

@anupcowkur anupcowkur self-requested a review January 6, 2025 09:01
Copy link
Contributor

@anupcowkur anupcowkur left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This is a legitimate UX issue and needs addressing.

The issue with the current PR is that some of our APIs require at least one selection to be present and will throw errors if no filter option is selected. It is invalid to have no selection for those filters in certain cases.

Suggestion: Make a "First" option instead of "None".

App version filter already has a "Latest" option to select only one app version so can be left as is. For the other two filter types you've modified, you can add a "First" option that will select the first item in the list. This should solve the problem of having to deselect everything one by one and make things easier.

@vunder
Copy link
Contributor Author

vunder commented Jan 6, 2025

The issue with the current PR is that some of our APIs require at least one selection to be present and will throw errors if no filter option is selected. It is invalid to have no selection for those filters in certain cases.

What is UI will not make any request to BE if nothing was selected?

@anupcowkur
Copy link
Contributor

anupcowkur commented Jan 6, 2025

The issue with the current PR is that some of our APIs require at least one selection to be present and will throw errors if no filter option is selected. It is invalid to have no selection for those filters in certain cases.

What is UI will not make any request to BE if nothing was selected?

When selection changes, we have to make a BE request.

For example, if you had selected items 1 and 2 and then hit "None", the frontend will have to request new data from backend since the selection is now different from what it was originally. If no request is made then old data will be displayed (which would be for items 1 & 2) leading to UI inconsistency.

@vunder
Copy link
Contributor Author

vunder commented Jan 6, 2025

What I ment is: what if not trigger onChangeSelected if nothing is selected?

    if (selected.length > 0) {
        onChangeSelected?.(selected);
    }

@anupcowkur
Copy link
Contributor

What I ment is: what if not trigger onChangeSelected if nothing is selected?

    if (selected.length > 0) {
        onChangeSelected?.(selected);
    }

We would run into the same problem as before. If something gets unselected, the data needs to be refreshed so that the selection matches the data in the UI so we would always have to fire the listener.

@vunder
Copy link
Contributor Author

vunder commented Jan 12, 2025

My proposal is following:

  useEffect(() => {
    if (!Array.isArray(selected) || selected.length > 0)
      onChangeSelected?.(selected);
  }, [selected]);

Basically - do not trigger selection change event if the selected state is invalid. In this case - do not trigger event if selected array data is empty

Suggestion: Make a "First" option instead of "None".
"First" does not helpful if you have dataset with is not "historical" (e.g. Device Manufacturer). And such option forces to have several additional steps when you just want to select some specific value:

  • click "First"
  • find necessary item and selected it
  • go to the beginning of the list to manually deselect first item

@anupcowkur
Copy link
Contributor

My proposal is following:

  useEffect(() => {
    if (!Array.isArray(selected) || selected.length > 0)
      onChangeSelected?.(selected);
  }, [selected]);

Basically - do not trigger selection change event if the selected state is invalid. In this case - do not trigger event if selected array data is empty

Suggestion: Make a "First" option instead of "None".
"First" does not helpful if you have dataset with is not "historical" (e.g. Device Manufacturer). And such option forces to have several additional steps when you just want to select some specific value:

  • click "First"
  • find necessary item and selected it
  • go to the beginning of the list to manually deselect first item

Understood the proposal. The issue still remains though - If some items are selected and then "None" is pressed, if we do not fire a change, then the UI will reflect the state of those previous items being selected and not the new state (nothing selected).

Regarding "First", it's not ideal so I think we will invest some time into making all the APIs accept no filters without throwing errors and then revisit this.

Tracking that issue here: #1725

@anupcowkur
Copy link
Contributor

We discussed this internally. We'll go ahead and merge this.

Just need one change: the commit format check is failing because "None" in the commit message is Camel case. If you can make it "none" and amend the commit, we can proceed with the merge once the lint check passes.

@vunder
Copy link
Contributor Author

vunder commented Jan 20, 2025

Just need one change: the commit format check is failing because "None" in the commit message is Camel case. If you can make it "none" and amend the commit, we can proceed with the merge once the lint check passes.

Since changes were pushed, amend produced an additional merge commit. I can create another branch

@vunder vunder requested a review from anupcowkur January 21, 2025 07:03
@anupcowkur
Copy link
Contributor

Just need one change: the commit format check is failing because "None" in the commit message is Camel case. If you can make it "none" and amend the commit, we can proceed with the merge once the lint check passes.

Since changes were pushed, amend produced an additional merge commit. I can create another branch

Sure. You'll have to close this PR and open a new one from that branch.

@vunder vunder closed this Jan 21, 2025
@vunder vunder deleted the none-filters-selector branch January 21, 2025 20:46
@vunder
Copy link
Contributor Author

vunder commented Jan 21, 2025

@anupcowkur Please check new one #1752

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.

"None" selection option in filters dropdown
2 participants