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] Improve examples #38398

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Aug 9, 2023

A quick polish on the examples. We could still push it further but no more bandwidth.

Implement some of #38204 (comment)

https://deploy-preview-38398--material-ui.netlify.app/material-ui/getting-started/example-projects/

@oliviertassinari oliviertassinari added the examples Relating to /examples label Aug 9, 2023
@@ -3736,7 +3736,7 @@ _Jul 4, 2022_

A big thanks to the 13 contributors who made this release possible. Here are some highlights ✨:

- 🐛 Fixed an issue causing Typescript errors when building a project with Material UI v5.8.6 (@michaldudak)
- 🐛 Fixed an issue causing TypeScript errors when building a project with Material UI v5.8.6 (@michaldudak)
Copy link
Member Author

Choose a reason for hiding this comment

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

Wrong capitalization

@@ -4,7 +4,7 @@ export default function PlayerFinal() {
return (
<iframe
title="codesandbox"
src="https://codesandbox.io/embed/github/mui/material-ui/tree/master/examples/base-ui-cra-tailwind-ts?hidenavigation=1&fontsize=14&view=preview"
src="https://codesandbox.io/embed/working-with-tailwind-css-dhmf8w?hidenavigation=1&fontsize=14&view=preview"
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +30 to 33
<link rel="preconnect" href="https://fonts.googleapis.com" />
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
<link
rel="stylesheet"
Copy link
Member Author

Choose a reason for hiding this comment

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

Best practice: See the installation instructions https://fonts.google.com/?preview.size=24&sort=popularity, to understand the win, the simplest is to compare the network requests with and without in https://www.webpagetest.org/. You will see it saves time by doing thins in parallel e.g. TLS handshake.

name: 'Create React App',
name: 'Vite.js',
Copy link
Member Author

Choose a reason for hiding this comment

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

More popular than CRA now

Comment on lines +140 to +142
data-ga-event-category="material-ui-example"
data-ga-event-label={example.name}
data-ga-event-action="click"
Copy link
Member Author

Choose a reason for hiding this comment

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

So we know which example to focus on

Comment on lines 18 to 21
```bash
# npm
npm install
npm run dev

# yarn
yarn
yarn dev

# pnpm
pnpm install
pnpm dev
```
Copy link
Member Author

Choose a reason for hiding this comment

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

Beginners use npm, the rest might use yarn or pnpm but are not juniors, so I don't think we really need these.

Comment on lines 1 to 3
'use client';

import * as React from 'react';
import createCache from '@emotion/cache';
Copy link
Member Author

Choose a reason for hiding this comment

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

To match the rest of the codebase, but React documents this with a blank line
https://react.dev/reference/react/use-client#use-client. Should we refactor all our codebase @mj12albert?

Comment on lines 11 to 14
<link
rel="stylesheet"
href="https://fonts.googleapis.com/css?family=Roboto:300,400,500,700&display=swap"
href="https://fonts.googleapis.com/css2?family=Inter:wght@300;500;600;700&display=swap"
/>
Copy link
Member Author

Choose a reason for hiding this comment

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

Joy UI doesn't use Roboto

Comment on lines 73 to 76
<link
rel="stylesheet"
href="https://fonts.googleapis.com/css?family=Inter:wght@300;400;500;600;700&display=swap"
href="https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&display=swap"
/>
Copy link
Member Author

Choose a reason for hiding this comment

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

This was broken, variables fonts are only in v2 https://developers.google.com/fonts/docs/css2

import { RemixBrowser } from 'remix';
import { RemixBrowser } from '@remix-run/react';
Copy link
Member Author

Choose a reason for hiding this comment

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

remix was complaining, they changed the imports

You can head back to the documentation, continuing browsing it from the [templates](https://mui.com/material-ui/getting-started/templates/) section.
You can head back to the documentation and continue by browsing the [templates](https://mui.com/material-ui/getting-started/templates/) section.
Copy link
Member Author

Choose a reason for hiding this comment

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

Sync with the JavaScript version, no reason to be different.

Comment on lines -18 to +20
Pro tip: See more <Link href="https://mui.com/getting-started/templates/">templates</Link> in
the MUI documentation.
Pro tip: See more{' '}
<Link href="https://mui.com/material-ui/getting-started/templates/">templates</Link> in the
MUI documentation.
Copy link
Member Author

Choose a reason for hiding this comment

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

301 link

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.

Love to see it!

@oliviertassinari oliviertassinari merged commit c2c1264 into mui:master Aug 9, 2023
21 checks passed
@oliviertassinari oliviertassinari deleted the docs-improve-examples branch August 9, 2023 19:08
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 9, 2023

I might have broken stuff, we will see.

I would have preferred not to open this PR, but I figured I could help differentiate there.

I'm quite curious to see the analytic. With @bharatkashyap we talked about expanding our CLI to start from templates. If we had this in Material UI, we could get a better idea of the most popular examples (assuming the CLI sends telemetry), hence the ones to polish the most. In any case, I think that the GA events will be enough.

@samuelsycamore
Copy link
Member

Maybe we should establish a kind of quarterly or biannual review of the examples directory to make sure everything is functional and up to date? You surfaced quite a few small things that none of us noticed, and that went unreported by the community as well. We should be more proactive about keeping these tidy, in any case, because a broken or buggy example would be an awful first impression to make. Analytics ought to give us a much better idea of what to prioritize.

@oliviertassinari
Copy link
Member Author

Maybe we should establish a kind of quarterly or biannual review of the examples directory to make sure everything is functional and up to date?

I'm not sure: when stuff gets broken, developers tend to report it. I would be inclined to have thorough tests of these examples as a one-off exercise & each time there is a proposed change. I rarely can spot everything in a single review, it takes me a couple of iterations to see all the things that could be improved, which at the end of the day impacts the DX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Relating to /examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants