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(Dropdown): render selected item rich content #5578

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Mar 9, 2020

This PR matches rich content in the selected item to match its appearance in the dropdown options list

Fixes #5576.
Fixes #5698.

Testing / Reviewing

Ensure the selected item appears and behaves the same as it does in the dropdown options list

@netlify
Copy link

netlify bot commented Mar 9, 2020

Deploy preview for carbon-components-react ready!

Built with commit d372d76

https://deploy-preview-5578--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Mar 9, 2020

Deploy preview for carbon-elements ready!

Built with commit d372d76

https://deploy-preview-5578--carbon-elements.netlify.com

@laurenmrice
Copy link
Member

The link for the option number needs to be link-01 color token

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @emyarod! Just one thing in case if you want to do; Would it worth using the new inner-component for each items in the dropdown, too?

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@emyarod
Copy link
Member Author

emyarod commented Mar 10, 2020

@asudoh do you have any suggestion on what to replace it with for the demo?

@emyarod emyarod force-pushed the 5576-render-rich-dropdown-content branch from 49ce876 to 0e93d5b Compare March 10, 2020 13:27
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

lgtm!

@emyarod emyarod force-pushed the 5576-render-rich-dropdown-content branch 2 times, most recently from 9bb6298 to 115cbe6 Compare March 10, 2020 21:43
@asudoh
Copy link
Contributor

asudoh commented Mar 10, 2020

@emyarod told me that this is ready to merge - Thanks @emyarod! (For the update of itemToElement/itemToString abstraction, too)

@emyarod emyarod force-pushed the 5576-render-rich-dropdown-content branch from b4fa99f to 9411e54 Compare March 10, 2020 22:24
@joshblack
Copy link
Contributor

joshblack commented Mar 10, 2020

Didn't we end up deciding against doing this in another PR?

#4977

Might be misinterpreting, let me know if so!

Related issue: #4913

@emyarod
Copy link
Member Author

emyarod commented Mar 11, 2020

I think this change is warranted as we are finding that users are hacking itemToString to render rich content in the selected dropdown item (https://ibm-cloudplatform.slack.com/archives/C2K6RFJ1G/p1583529047246200), and this seems to be a a desired UX feature. it looks like the initial PR was closed due to concerns about potential a11y violations in custom content but that seems to be an application specific issue rather than an issue with the component behavior

@joshblack
Copy link
Contributor

@emyarod would we want to support arbitrary rendering in the field, though? It seems like a situation where we would want to constrain markup to only a string in order to stay consistent with visual guidelines along with making sure accessibility is not a concern (in the case of rendering an interactive widget in the field position)

@emyarod
Copy link
Member Author

emyarod commented Mar 12, 2020

@joshblack yes I believe so, given that design has expressed that this should be the expected behavior (dropdown option appears the same when it is the selected item) and that users are currently hacking itemToString to achieve this. as far as a11y concerns go, in this situation it is on the users to not violate a11y especially since they would have more control over what content to place in the dropdown component. but we are not shipping a component with any violations out of the box

@joshblack
Copy link
Contributor

Just a quick note, it seems like the storybook environment may be off?
demo

Separately, should we using a link as the example? It seems like this might not pass AVT2 since the link is inoperable?

demo


I don't know if there is alignment around this feature with design, I believe they were only made aware of itemToElement through this PR. It seems like this feature in a broader sense enables unintended behavior in these controls as noted above. In particular, if we render something like a tooltip it causes problems not only in the option but also in the field position. This may also be unintended behavior if we move this method over as an individual who renders a tooltip in the options list may not want it in the field position.

Overall, this functionally seems to enable teams to create inaccessible and inconsistent UIs that often create a worse experience for users. As a result, I don't think this is functionality that we want to extend into the field position and potentially cause additional problems with this component.

@emyarod emyarod force-pushed the 5576-render-rich-dropdown-content branch from a5439c2 to d52afaf Compare March 13, 2020 13:12
@emyarod
Copy link
Member Author

emyarod commented Mar 13, 2020

Storybook issue should be resolved now. It seems that the concern is about what our users could potentially come up with. Since we already allow rich content in the options menu we can at least provide the option to render the selected item unchanged as an escape hatch, especially after seeing previous requests for this and how users are currently hacking around component limitations to implement this. I don't see the benefit to attempting to limit this when users are already implementing this behavior (in a broken fashion) with the current component API

@emyarod emyarod force-pushed the 5576-render-rich-dropdown-content branch from d52afaf to c5d5e35 Compare March 13, 2020 13:34
@joshblack
Copy link
Contributor

It seems that the concern is about what our users could potentially come up with.

The idea here is that there seems to be no defining use-case for this functionality and that providing it could make it easier to build inaccessible or inconsistent experiences.

Some of the cases that popped up here that we wouldn't want to enable:

  • Custom colors in the field or listbox position (inconsistent experience)
  • Tooltips in the field or listbox position (inaccessible experience)
  • Links in the field or listbox position (inaccessible experience)
  • Icons in the field or listbox position (inconsistent experience)

In addition, reusing the API for both option and field could be considered a bug for people who may only want to render the rich content in one place but not the other (even if doing so would provide an inconsistent or inaccessible experience)

Similarly, we should not use a link in our example regardless because it seems to fail AVT2. This type of situation is what we want to avoid. If folks want to hack around stuff to do this, they are well within their rights to do so. It doesn't mean that we make this a common or happy path when it doesn't align with a use-case of this component or the experience that we want folks to leverage while shipping product functionality.

@joshblack
Copy link
Contributor

joshblack commented Mar 31, 2020

Closing based on above and resolution from support channel 👍 Feel free to reach out if this should be re-opened!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown can not display selected element (string is fine) Render rich content as dropdown selected item
6 participants