-
-
Notifications
You must be signed in to change notification settings - Fork 267
[docs-infra] Fix a11y and HTML errors on baseline docs page #2718
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: master
Are you sure you want to change the base?
Conversation
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Bundle size report
|
oliviertassinari
left a comment
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.
Extra context added. Normally, those changes lead to no visual diff (that are not bug fixes).
| export function Code({ className, ...props }: React.ComponentProps<'code'>) { | ||
| return <code className={clsx('Code', className)} {...props} />; | ||
| export function Code(props: React.ComponentProps<'code'>) { | ||
| return <code {...props} className={clsx('Code', props.className)} />; |
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.
Avoid JS object destructuring; it's faster. This components is rendered a bunch on the page.
| } | ||
|
|
||
| export function Panel({ className, children, ...props }: React.ComponentPropsWithoutRef<'div'>) { | ||
| export function Panel({ className, children, ...other }: React.ComponentPropsWithoutRef<'div'>) { |
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.
Usual naming convention in the codebase (move closer to have a single convetion in use, to help others navigate it).
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.
In the repo (package and docs) ...props is still by far the most common though
| event.key === 'a' && | ||
| (event.metaKey || event.ctrlKey) && | ||
| (event.ctrlKey || event.metaKey) && | ||
| String.fromCharCode(event.keyCode) === 'A' && |
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.
The logic was wrong; it ignored, e.g. Dvorak keyboard. This is a straight copy & paste from Material UI, not related to the PR, but I saw it, so why not? For those who wonder, at the root, it's an issue with the web standard.
| <Accordion.Scrollable gradientColor="var(--color-gray-50)"> | ||
| <pre {...props} className="text-xs p-0 m-0" style={{ backgroundColor: 'none' }} /> | ||
| <Accordion.Scrollable component="div" gradientColor="var(--color-gray-50)"> | ||
| <pre {...props} className="text-xs p-0 m-0" style={{ backgroundColor: undefined }} /> |
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.
background-color: none doesn't exist: https://stackoverflow.com/questions/8739665/is-background-colornone-valid-css. Instead, remove the inline style applied.
| </DescriptionList.Details> | ||
| </DescriptionList.Item> | ||
| )} | ||
|
|
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.
Avoid blank lines in JSX; those always feel arbitrarily placed with no meaning (component separation usually works a lot better to convey context).
| p: (props) => <p {...props} />, | ||
| p: (props) => props.children, |
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.
We don't need an extra <p> when we inline code, so flatten it.
c440fe7 to
6649a9c
Compare
| const { | ||
| children, | ||
| className, | ||
| component: Component = 'span', |
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 don't think this component prop is necessary; is it because div is shorter than span? If so this could just always be a div
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.
<Accordion.Scrollable> is used as a child of a <summary> which only allows phrasing content, so it can't be a div, a span works.
But <Accordion.Scrollable> is also used as a parent of a <pre> element, which only allows its parent to be a flow content, so it can't be a span; a div works.
So it needs to be parameterized.
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.
How about tag instead of component? We already use this convention a bit in the package, e.g.
| tag="button" |
| rehypePlugins: rehypeSyntaxHighlighting, | ||
| useMDXComponents: () => ({ | ||
| code: ExpandedCode, | ||
| ...inlineMdxComponents, |
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.
Why does this need to be spread here now?
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 expect that all use of createMdxComponent() must extend the default typography set, so:
diff --git a/docs/src/mdx/createMdxComponent.ts b/docs/src/mdx/createMdxComponent.ts
index 6314f9b9..98c99684 100644
--- a/docs/src/mdx/createMdxComponent.ts
+++ b/docs/src/mdx/createMdxComponent.ts
@@ -1,5 +1,6 @@
import * as jsxRuntime from 'react/jsx-runtime';
import { evaluate, EvaluateOptions } from '@mdx-js/mdx';
+import { mdxComponents } from '../mdx-components';
export async function createMdxComponent(
markdown = '',
@@ -8,6 +9,7 @@ export async function createMdxComponent(
) {
const { default: Component } = await evaluate(markdown, {
...jsxRuntime,
+ ...mdxComponents,
...options,
} as EvaluateOptions);
return Component;It's only about a step to make this migration easier.
A quick, fun one.
I see this as a regression in Base UI vs. MUI Base https://v6.mui.com/base-ui/react-slider/ that only has errors from the components (or almost).
I can't find one clear PR that broke this in Base UI, it seems to have been ignored from the start, so only a pattern of not considering those errors in the Definition of Done. Which I think we can turn into: mui/mui-public#682.
Which I think we can turn into: mui/mui-public#683.
After this PR change, we only have the bugs that our components have. For example, on the Slider docs page:
https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-2718--base-ui.netlify.app%2Freact%2Fcomponents%2Fslider
Someone could now easily spot the issues and fix them:
aria-valueminis redundant, to delete.aria-valuemaxis redundant, to delete."". This is not the same: an input without a name doesn't get sent, one with an empty string gets sent with an empty string as key.cc @mj12albert
Preview: https://deploy-preview-2718--base-ui.netlify.app/react/components/slider