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

[base-ui][Select] Fix display of selected Options with rich content #40689

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Jan 19, 2024

When Select's Options had rich content (= more than a plain string), such as an icon or an image, selecting such an option did not display properly. The effect was as if nothing was selected.

See the "Select a component" Select:
before: https://deploy-preview-40683--material-ui.netlify.app/base-ui/
after: https://deploy-preview-40689--material-ui.netlify.app/base-ui/

@michaldudak michaldudak added bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Jan 19, 2024
@mui-bot
Copy link

mui-bot commented Jan 19, 2024

Netlify deploy preview

https://deploy-preview-40689--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 9a914b3

@@ -43,7 +43,7 @@ const InnerOption = React.memo(
// If `label` is not explicitly provided, the `children` are used for convenience.
// This is used to populate the select's trigger with the selected option's label.
const computedLabel =
label ?? (typeof children === 'string' ? children : optionRef.current?.innerText);
label ?? (typeof children === 'string' ? children : optionRef.current?.textContent?.trim());
Copy link
Member

Choose a reason for hiding this comment

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

How does this affect/fix the demo? Testing it out and getting the same value for both innerText and textContent:

Screen.Recording.2024-01-19.at.10.52.58.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

In the "before" demo, the Select is initially "collapsed" (too short).

The difference between innerText and textContent (that I learned today) is that innerText takes display properties into consideration. So if an option is hidden (as it initially is), innerText will return ''. textContent, on the other hand always returns what's defined in the DOM node, regardless of visibility.

Copy link
Member

Choose a reason for hiding this comment

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

Aah, right, it's collapsed, so it's hidden, and innerText doesn't return it, gotcha!

@michaldudak michaldudak merged commit 635db56 into mui:master Jan 19, 2024
22 checks passed
@michaldudak michaldudak deleted the rich-option-display-fix branch January 19, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants