-
-
Notifications
You must be signed in to change notification settings - Fork 268
[docs] New props table #2135
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] New props table #2135
Conversation
commit: |
Bundle size report
|
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ee2882d to
c06723b
Compare
| if (['boolean', 'string', 'number'].includes(type)) { | ||
| return type; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. The [...].includes(...) syntax is bad for performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this is just a throwaway ~
But should we generate the short prop type here or via the api extractor? @michaldudak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If API extractor returns an IntrinsicNode, we should use it as is.
If the returned type has both the name an either types (like a union or intersection) or properties (object type), use the name as a short type and drill into the details for the expanded view.
d40518c to
0f4851e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should avoid repeating the default value in the expanded details. It adds noise if we repeat information, and noise is not great for UX.
And I wonder if we shouldn't mark the Type in the expanded details as something like Type (detailed), or inversely mark the column header as Type (simplified), to convey that there's possibly more information in the expanded details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if reducing noise is the goal, then I would remove the default from the summary and only have it in the panel.
In the screenshot below, the visual noise is coming from the repetition of the word "default".
So I see 3 options:
- Remove the default from the summary, and just have it in the expanded panel. Of course, the trade-off is that you can't see the default at a glance.
- Add a new column for default.
- Just leave it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Sounds like a worst UX, there's less information available without clicking. When I refer to a props table, it's nice to be able to keep it open as a reference while I write code and see everything at a glance.
- That's the way it was before, and it left less place for the type description, and in some cases it was nearly all empty. Tbh I suggested merging type & default in a single column because I was hoping to have more space to show the type, but now that long function types are always simplified to
functionand hidden, it's less relevant, but imho hidding the function types isn't necessary. - Doesn't feel too bad. We could also find a terser notation, e.g. name the column
Type [Default]and use values likeboolean [false]. Or integrate the default to the name column, e.g.defaultChecked = false, but we have a lot of unused space in the type column.
edit: Orboolean [=false], somewhat similar to jsdoc's[name=default]notation.
Sidenote but there's still default: undefined entries, we should not display the default if there is none or if it's undefined.
| <TableCode className="text-navy">{name}</TableCode> | ||
| {prop.type && ( | ||
| <span className="flex items-baseline gap-4 text-sm"> | ||
| <ShortPropType /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about hiding the detailed type in the details in some cases. For render & className it makes sense to always hide it, but for other cases (like below) I think it might degrade the UX a bit to have to click to see the full type. I wonder if we could try to hide it based on its length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice for mouse users, but keyboard-centric users (which are probably higher in proportion among devs) don't have that, and requiring a hover is not that far from requiring a click. I still think we should use the space if we have the space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @colmtuite , this continues from #2120 (comment)
I toned down the shadow, but there's still the issue of the open panel background being very similar to the hover state of the trigger
015c0ac to
b0fdd83
Compare
|
Needs #2104 to generate the json in the new format |
bd4384c to
b20172b
Compare
|
Seems like it shouldn't open while trying to select text. (The Accordion example made this impossible, but that's not accessible) Screen.Recording.2025-07-03.at.7.06.44.pm.mov |
When clicking the text selection, should it cancel the text selection only or toggle the element as well? Screen.Recording.2025-07-07.at.4.47.18.PM.movOr should we use |
66b69cf to
8433206
Compare
|
The main issue with disabling text selection is that you can't copy the name of the prop when trying to select it. Double clicking also allows the entire text to be selected. Maybe trying to click on text shouldn't open it, but everywhere else is fine |
8433206 to
07d4cde
Compare
69c9a8f to
af8e1dc
Compare
|
@mj12albert I mentioned setting Screen.Recording.2025-07-22.at.07.25.47.mov |
I updated it to 0, what about in the mobile layout do you want the scroll margin to make the trigger flush against the header? @colmtuite
|
|
@mj12albert yea, looks nice 👌 |
|
For aligning the mobile layout of the various tables, the triggers all use Also added the overscroll graphic to the trigger as it could overflow in some cases: scrollable.movAny further feedback or is it good to go? @colmtuite |






Preview: https://deploy-preview-2135--base-ui.netlify.app/react/components/toggle
Uses native
<details>+<summary>for the new props table design; also applies to CSS vars and data attrs tables where the popover is replaced with a<details>.Notable differences from Accordion:
hidden="until-found"by default when supported, no need to fight React 👍Closes #1752