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] Convert alpha component docs to new docs template #392

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

colmtuite
Copy link

@colmtuite colmtuite commented May 2, 2024


{{"demo": "UnstyledNumberFieldIntroduction", "defaultCodeOpen": false, "bg": "gradient"}}
Base UI components are all available as a single package.
Copy link

Choose a reason for hiding this comment

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

🚫 [vale] reported by reviewdog 🐶
[MUI.MuiBrandName] Use a non-breaking space (option+space on Mac, Alt+0160 on Windows or AltGr+Space on Linux, instead of space) for brand name ('Base UI' instead of 'Base UI')


{{"demo": "UnstyledSwitchIntroduction", "defaultCodeOpen": false, "bg": "gradient"}}
Base UI components are all available as a single package.
Copy link

Choose a reason for hiding this comment

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

🚫 [vale] reported by reviewdog 🐶
[MUI.MuiBrandName] Use a non-breaking space (option+space on Mac, Alt+0160 on Windows or AltGr+Space on Linux, instead of space) for brand name ('Base UI' instead of 'Base UI')


{{"demo": "UnstyledCheckboxIntroduction", "defaultCodeOpen": false, "bg": "gradient"}}
Base UI components are all available as a single package.
Copy link

Choose a reason for hiding this comment

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

🚫 [vale] reported by reviewdog 🐶
[MUI.MuiBrandName] Use a non-breaking space (option+space on Mac, Alt+0160 on Windows or AltGr+Space on Linux, instead of space) for brand name ('Base UI' instead of 'Base UI')

@michaldudak michaldudak changed the title Convert alpha component docs to new docs template [docs] Convert alpha component docs to new docs template May 2, 2024
@michaldudak michaldudak added the docs Improvements or additions to the documentation label May 2, 2024
@michaldudak
Copy link
Member

Maybe it's something to address separately (and later): I find the style of these three components' demos quite different. It would be great if we restyled them so they feel like they belong to the same design system.

@colmtuite colmtuite requested a review from michaldudak May 2, 2024 08:45
@colmtuite
Copy link
Author

colmtuite commented May 2, 2024

I find the style of these three components' demos quite different.

@michaldudak Yep the design consistency, design quality, CSS consistency, and CSS quality needs to be improved. I'll do it in a separate PR.

@colmtuite colmtuite requested a review from michaldudak May 2, 2024 10:13

## Accessibility

Ensure the number field has an accessible name via a `label` element.
Ensure the number field has an accessible name via a `<label/>` element.
Copy link
Contributor

Choose a reason for hiding this comment

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

<label> rather than <label/> is used for the other documents. It would be good to have a code example like they do as well here.

Comment on lines +220 to +222
```jsx
<NumberField.Input render={(props) => <MyCustomInput {...props} />}> />
```
Copy link
Contributor

Choose a reason for hiding this comment

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

We should show the two methods now that we support them:

<NumberField.Input render={<MyCustomInput />} />

This is where it's important to mention that the components need to forwardRef and spread all props to the underlying DOM element ("be open for extension")

(This one also has a stray > that I've removed)

<NumberField.Input render={(props) => <MyCustomInput {...props} />} />

Copy link
Author

Choose a reason for hiding this comment

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

Since this impacts most components and is duplicate content, I'm gonna move this into the global page in the "getting started" section. We can still include this little section on each page with a link to the more detailed page, but I think we should avoid duplicating this large block on every component page.

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Just a few nitpicks while reading the new doc

docs/data/base/components/number-field/number-field.md Outdated Show resolved Hide resolved
docs/data/base/components/number-field/number-field.md Outdated Show resolved Hide resolved

{{"component": "modules/components/ComponentLinkHeader.js", "design": false}}

{{"component": "modules/components/ComponentPageTabs.js"}}

## Introduction
{{"demo": "UnstyledNumberFieldIntroduction", "defaultCodeOpen": false, "bg": "gradient"}}
Copy link
Member

Choose a reason for hiding this comment

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

What is the logic behind the style of each CSS technology in this demo?
On the checkbox we have:

  • MUI System: blue
  • Tailwind CSS: pink
  • Plain CSS: grey

Here we have:

  • MUI System: grey
  • Tailwind CSS: darker grey
  • Plain CSS: grey

I personally think having the color switch when you change CSS technology is a great visual indication that something did change.
And having coherent colors throughout the components would be nice.

Copy link
Author

Choose a reason for hiding this comment

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

Will redesign the demos in a separate PR.

import * as Switch from '@base_ui/react/Switch';
```

### Anatomy
## Anatomy
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 miss the equivalent of these section

Same for Checkbox

Copy link
Author

Choose a reason for hiding this comment

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

Will consider for a separate PR.

@@ -84,18 +84,6 @@ Though not required, you can add the `value` prop to the Tab and Tab Panel to
</Tabs.Root>
Copy link
Member

Choose a reason for hiding this comment

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

Not strictly related to this PR, but having a demo on the "Orientation" section instead of just a text would be great

Copy link
Author

Choose a reason for hiding this comment

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

We're making an effort to minimise the page length. I think this is less important for an unstyled lib, since there's nothing really to see here except the orientation changing. It only gets interesting when you apply a height restriction, which would only happen in styled land.

So I'll skip this for now but will keep it in mind and maybe revisit once we make other unrelated docs improvements, and get a better sense of how many demos we can add before the pages feel too long.

Copy link
Member

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

These are looking really good! This review is mostly about style and formatting consistency.

@@ -9,108 +9,78 @@ waiAria: https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/

# Checkbox

<p class="description">Checkboxes give users binary choices when presented with multiple options in a series.</p>
<p class="description">Checkbox give users a binary choice between multiple options in a series.</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">Checkbox give users a binary choice between multiple options in a series.</p>
<p class="description">Checkbox gives users a binary choice between multiple options in a series.</p>

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 I prefer it in plural form though

Suggested change
<p class="description">Checkbox give users a binary choice between multiple options in a series.</p>
<p class="description">Checkboxes give users a binary choice between multiple options in a series.</p>

Copy link
Author

Choose a reason for hiding this comment

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

Will leave it in singular form because we need to apply either singular or plural to all components here, and we run into issues with plural. Fixed the typo though.


- **Forward all props**: Your component should spread all props to the underlying element.
- **Forward the `ref`**: Your component should use [`forwardRef`](https://react.dev/reference/react/forwardRef) to ensure the Checkbox components can access the element via a ref.
Checkbox is composed of two components.
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
Checkbox is composed of two components.
Checkbox is composed of two components:

```

### Indeterminate state
## Indeterminate state

To make the checkbox indeterminate, add the `indeterminate` prop to override the appearance of the checkbox. The checkbox remains in an indeterminate state regardless of user interaction until set back to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Always capitalize component names

Suggested change
To make the checkbox indeterminate, add the `indeterminate` prop to override the appearance of the checkbox. The checkbox remains in an indeterminate state regardless of user interaction until set back to `false`.
To make the Checkbox indeterminate, add the `indeterminate` prop to override the appearance of the Checkbox. The Checkbox remains in an indeterminate state regardless of user interaction until set back to `false`.


To make the checkbox indeterminate, add the `indeterminate` prop to override the appearance of the checkbox. The checkbox remains in an indeterminate state regardless of user interaction until set back to `false`.

{{"demo": "UnstyledCheckboxIndeterminate.js"}}

An indeterminate checkbox's main use case is representing the state of a parent checkbox where only some of its children are checked:
The primary use case for an indeterminate checkbox is representing the state of a parent checkbox where only some of its children are checked.
Copy link
Member

Choose a reason for hiding this comment

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

just pointing this out for the sake of clarification: lowercase "checkbox" works in this context since we're discussing the general concept rather than the specific Base UI component.

### Anatomy
## Anatomy

NumberField is implemented using a collection of related components:
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
NumberField is implemented using a collection of related components:
Number Field is implemented using a collection of related components:

Copy link
Author

Choose a reason for hiding this comment

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

If we're referencing the actual component—as we want to emphasise via capitalisation—we should omit the space, since we're not talking about a "number field" in general, we're talking about Base UI's "NumberField".

Copy link
Member

Choose a reason for hiding this comment

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

The convention we've established is to treat these component names as proper nouns, capitalized and with spaces between words, to differentiate between The Base UI Component and the concept of such a component. Material UI used to (inconsistently) style these names like NumberField but we've moved to Number Field over the last couple years. This doc explains the conventions: https://www.notion.so/mui-org/Style-conventions-for-MUI-components-and-products-dff7d2d7d1ba4a4abd47f48cf345b800

Copy link
Author

Choose a reason for hiding this comment

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

Ok I updated it

@@ -190,46 +189,40 @@ The pointer is locked while scrubbing, allowing the user to scrub infinitely wit
</NumberField.ScrubArea>
```

In your CSS, ensure any `label` elements inside the `ScrubArea` specify `cursor: unset`. You can rotate the above macOS-style cursor 90 degrees using a `transform` style.
In your CSS, ensure any `<label/>` elements inside the `ScrubArea` specify `cursor: unset`. You can rotate the above macOS-style cursor 90 degrees using a `transform` style.
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
In your CSS, ensure any `<label/>` elements inside the `ScrubArea` specify `cursor: unset`. You can rotate the above macOS-style cursor 90 degrees using a `transform` style.
In your CSS, ensure any `<label>` elements inside the Scrub Area specify `cursor: unset`.
You can rotate the above macOS-style cursor 90 degrees using a `transform` style.

@@ -190,46 +189,40 @@ The pointer is locked while scrubbing, allowing the user to scrub infinitely wit
</NumberField.ScrubArea>
```

In your CSS, ensure any `label` elements inside the `ScrubArea` specify `cursor: unset`. You can rotate the above macOS-style cursor 90 degrees using a `transform` style.
In your CSS, ensure any `<label/>` elements inside the `ScrubArea` specify `cursor: unset`. You can rotate the above macOS-style cursor 90 degrees using a `transform` style.

:::info
In Safari, the pointer is not locked. However, this doesn't affect the ability to scrub infinitely.
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
In Safari, the pointer is not locked. However, this doesn't affect the ability to scrub infinitely.
In Safari, the pointer is not locked.
However, this doesn't affect the ability to scrub infinitely.


## Accessibility

Ensure the number field has an accessible name via a `label` element.
Ensure the number field has an accessible name via a `<label/>` element.
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
Ensure the number field has an accessible name via a `<label/>` element.
Ensure the Number Field has an accessible name via a `<label>` element.

Copy link
Author

Choose a reason for hiding this comment

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

I've already changed this based on conflicting feedback, but will go with this suggested change since this is what I had initially. We can get our ducks in a row later on I guess.

### Anatomy
## Anatomy

Switch is composed of two components.
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
Switch is composed of two components.
Switch is composed of two components:


The Switch component is composed of a root that houses one interior slot—a thumb:
- `<Switch.Root />` renders a `<button>`.
- `<Switch.Thumbs />` renders a `<span>` for providing a visual indicator.
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
- `<Switch.Thumbs />` renders a `<span>` for providing a visual indicator.
- `<Switch.Thumb />` renders a `<span>` for providing a visual indicator.

@colmtuite
Copy link
Author

@flaviendelangle @samuelsycamore Thanks for the feedback. I've pushed a commit incorporating your changes.

@colmtuite
Copy link
Author

Any approvals going? @atomiks @samuelsycamore


The `ScrubArea` subcomponent lets users scrub the value with their pointer as a faster alternative to the stepper buttons. This is useful in high-density UIs, such as an image editor that changes the width, height, or location of a layer:
The `ScrubArea` subcomponent lets users increment/decrement the value via a click+drag interaction with pointer, as a faster alternative to the stepper buttons. This is useful in high-density UIs, such as an image editor that changes the width, height, or location of a layer. You could wrap an icon or a `<label/>` in the `ScrubArea` component.
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 `ScrubArea` subcomponent lets users increment/decrement the value via a click+drag interaction with pointer, as a faster alternative to the stepper buttons. This is useful in high-density UIs, such as an image editor that changes the width, height, or location of a layer. You could wrap an icon or a `<label/>` in the `ScrubArea` component.
The Scrub Area subcomponent lets users increment and decrement the value via a click-and-drag interaction with pointer, as a faster alternative to the stepper buttons.
This is useful in high-density UIs, such as an image editor that changes the width, height, or location of a layer.
You could wrap an icon or a `<label>` in the Scrub Area component.

Copy link
Member

Choose a reason for hiding this comment

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

@flaviendelangle is this description of scrubbing more clear to you?

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.

None yet

5 participants