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.

Sign up for GitHub

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

Jump to bottom

FilterItem: value prop #1505

Open
1 task done
rafarubim opened this issue Feb 8, 2024 · 2 comments · May be fixed by #1982
Open
1 task done

FilterItem: value prop #1505

rafarubim opened this issue Feb 8, 2024 · 2 comments · May be fixed by #1982
Labels
dev Indicates that the issue or pull request involves engineering considerations

Comments

@rafarubim
Copy link

Support or Feedback

  • I've searched for any related issues/questions on Github before submitting this request.

Context

Shoreline's component FilterItem receives a value and a children prop (it works like the <select>/<option> HTML tags).

image

The children text is presented at each of the Filter popover options. But after an option is "selected" then "applied", the value prop is what is actually shown by the Filter button (in the following image, look at the Roles: 123 text):

image

Suggestion

My suggestion is to change this behavior, to show the children prop text instead:

image

This would make the <Filter>/<FilterOption> behavior work more like HTML's <select>/<option> tags, making the children prop related to the visuals and the value prop related to React's controlled state. This would allow us to use ID strings in the value prop without harming UI visuals - and without constantly converting IDs<=>strings within controlled filters.

Additional Suggestion

If the previous suggestion is accepted, I would also suggest accepting the type number in the value prop, so there's no need for constant conversion from/to numeric IDs (id.toString()/Number.parseInt(value, 10)).``
@matheusps
Copy link
Contributor

matheusps commented Feb 8, 2024 •
edited
Loading

I agree with the suggestion for the children! Regarding numbers, I don't. I think that is better to stick with string, which is the standard for inputs in HTML, and convert to a number if you need to.

@davicostalf davicostalf changed the title Shoreline FilterItem component value prop Filter: FilterItem component value prop Feb 9, 2024
@davicostalf davicostalf changed the title Filter: FilterItem component value prop FilterItem: value prop Feb 9, 2024
@davicostalf davicostalf added the dev Indicates that the issue or pull request involves engineering considerations label Jul 23, 2024
@davicostalf
Copy link
Contributor

davicostalf commented Aug 15, 2024 •
edited
Loading

@matheusps can you update the issue title with a definition of what should be done?

@beatrizmilhomem beatrizmilhomem moved this to Discussing in Shoreline Aug 16, 2024
viniciuslagedo added a commit that referenced this issue Sep 27, 2024
@viniciuslagedo
fix(filter-value): show children text content instead of value
4566cc7 
Use the inner text from the selected item or from the first selected item instead of the value
property. It makes more sense because the children prop is more related to visual purposal

fix #1505
@viniciuslagedo viniciuslagedo linked a pull request Sep 27, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dev Indicates that the issue or pull request involves engineering considerations
Projects
Shoreline
Status: Discussing
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(filter-value): show children text content instead of value vtex/shoreline
3 participants
@matheusps @davicostalf @rafarubim