-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[joy-ui][docs] Fix h1 template #40017
[joy-ui][docs] Fix h1 template #40017
Conversation
Netlify deploy previewhttps://deploy-preview-40017--material-ui.netlify.app/ Bundle size report |
@@ -7,13 +7,11 @@ import Typography from '@mui/joy/Typography'; | |||
import Button from '@mui/joy/Button'; | |||
import Stack from '@mui/joy/Stack'; | |||
|
|||
// Icons import |
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.
Seems to be noise and not consistently applies between files
<Box | ||
sx={{ | ||
alignItems: 'center', | ||
gap: 1, | ||
}} | ||
> | ||
<Box sx={{ alignItems: 'center', gap: 1 }}> |
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.
Try to keep the line count as low as possible. This is often a concern relative to Tailwind CSS: how the screen is not tall enough to see the structure of the component.
@@ -203,7 +201,6 @@ export default function EmailContent() { | |||
> | |||
to | |||
</Typography> | |||
|
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 have no space for JSX line breaks in the coding style.
function SideDrawer( | ||
props: BoxProps & { onClose: React.MouseEventHandler<HTMLDivElement> }, |
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.
Use function Foo(props)
convention to "flag" React components.
export const closeMessagesPane = () => { | ||
export function closeMessagesPane() { |
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.
Convention, no arrow function at the top level scope of a file
const status = useScript(`https://unpkg.com/feather-icons`); | ||
const status = useScript('https://unpkg.com/feather-icons'); |
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.
Convention, single quote when possible.
my: 1, | ||
mb: 1, |
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.
Make the head of https://mui.com/joy-ui/getting-started/templates/profile-dashboard/ and https://mui.com/joy-ui/getting-started/templates/order-dashboard/ to be at the same position.
@zanivan One thing I noticed that is frustrating is the layout shift with the tabs: Screen.Recording.2023-11-26.at.18.27.43.movIn https://dashboard.workos.com/environment_01H69ZHYYTR835G2GQ78SPZT5A/configuration/directory-sync, they (WorkOK, Radix Theme, same thing) render two labels to avoid the layout shift: GitHub solutions to the problem look significantly better to me: Maybe changing the font weight isn't the best idea 😁 |
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.
Wow! Really appreciated all the detailed feedback and insights 🙏
Regarding the tab issue, I'll open a PR for the fix 😉
Fix the issues reported in https://app.ahrefs.com/site-audit/3524616/93/data-explorer?columns=pageRating%2Curl%2Ctraffic%2ChttpCode%2Cdepth%2Ch1%2Ch1Length%2CnrH1%2Ccompliant&filterId=e29b87ac6311cffd6273e3758dd92cd7&issueId=838c947a-001a-11e8-823b-001e67ed4656.
The solution is to add
component="h1"
wherever relevant.