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

[docs] Move SelectUnstyled docs to the Base space #31816

Merged
merged 16 commits into from
Mar 29, 2022

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Mar 15, 2022

Moved the SelectUnstyled, MultiSelectUnstyled, OptionUnstyled, and OptionGroupUnstyled docs to the Base space.

Preview - https://deploy-preview-31816--material-ui.netlify.app/base/react-select/

@michaldudak michaldudak added the docs Improvements or additions to the documentation label Mar 15, 2022
@mui-bot
Copy link

mui-bot commented Mar 15, 2022

No bundle size changes

Generated by 🚫 dangerJS against 63e2f72

@siriwatknp
Copy link
Member

@michaldudak You might have to do the same with https://github.com/mui/material-ui/pull/31417/files#diff-0f62cd80e39c6c4c31bfbd4d836a0e888559f35b05d9d1d833db0e815d94d6f3 so that the yarn docs:api works correctly.

@michaldudak michaldudak marked this pull request as ready for review March 15, 2022 13:49
@siriwatknp
Copy link
Member

siriwatknp commented Mar 15, 2022

2 points for discussion:

  1. Should all the headings have the same level (currently most of the demos are nested in "The SelectUnstyled component" which affects the search to be in the lower rank)?

Screen Shot 2565-03-15 at 23 13 40

  1. I think the heading should contain only keyword. For example:

"The SelectUnstyled component" should be just "Select unstyled"
"Grouping" should be "Grouping options" (to be more explicit)
"Customizing the selected value appearance" should be just "Selected value appearance"

docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
@michaldudak
Copy link
Member Author

michaldudak commented Mar 16, 2022

Should all the headings have the same level

@siriwatknp It doesn't feel right. The "Basic usage", "Controlled select", etc. are about the unstyled component, so to me, it makes sense to have them nested.

I think the heading should contain only keyword.

All right, no problem.

michaldudak and others added 2 commits March 16, 2022 13:14
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
The `SelectUnstyled` component accepts generic props.
Due to TypeScript limitations, this may cause unexpected behavior when wrapping the component in `forwardRef` (or other higher-order components).
In such cases, the generic argument will be defaulted to `unknown` and type suggestions will be incomplete.
To avoid this, you can manually cast the resulting component to the correct type, as shown in the demo above.
Copy link
Member

Choose a reason for hiding this comment

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

How about we show a short code snippet of this instead of asking developers to open the previous demo?

Copy link
Member

Choose a reason for hiding this comment

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

Good call!


{{"demo": "UnstyledSelectMultiple.js", "defaultCodeOpen": false}}

## The useSelect hook
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## The useSelect hook
## The `useSelect` hook

?

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to avoid back ticks in headers. At least, I think that's the way it already is elsewhere in the docs. But maybe we don't have a consistent pattern established.

Co-authored-by: Marija Najdova <mnajdova@gmail.com>
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Two last comments from my side, the rest looks good


# Select

<p class="description">The SelectUnstyled and MultiSelectUnstyled components let you create menus of options for users to choose from.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p class="description">The SelectUnstyled and MultiSelectUnstyled components let you create menus of options for users to choose from.</p>
<p class="description">The select components allow developers to create menus of options for users to choose from.</p>

We are already mentioning below the name of the components, so it's redundant. Also, to keep consistent with other docs pages, I have rephrased a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

According to #31850 (comment), "allow" is not the best word to use here. How about "The select components let you create..."

Also, I'm not sure about the word "menus" here. I understand it may be used in a broader sense than "a menu component", but I'm afraid it could confuse developers and make them wonder which component should be used to actually implement a menu control.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then we need to revisit all unstyled docs in the end and update this, some may have been merged already if I am not mistaken.

For the menus, I had the same thought, I was thinking list of options may be better. Similar terminology is used in wai aria too - https://www.w3.org/TR/wai-aria-practices-1.1/examples/listbox/listbox-collapsible.html (fi this is the equivalent)

docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
docs/data/base/components/select/select.md Outdated Show resolved Hide resolved
michaldudak and others added 2 commits March 21, 2022 11:20
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 21, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 22, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 25, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 29, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 29, 2022
@michaldudak michaldudak merged commit 3905693 into mui:master Mar 29, 2022
@michaldudak michaldudak deleted the select-docs branch March 29, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants