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

[material-ui][Typography] Deprecate paragraph prop #42383

Merged
merged 24 commits into from
Jul 23, 2024

Conversation

walston
Copy link
Contributor

@walston walston commented May 24, 2024

Resolves #42382, resolves #16926

@mui-bot
Copy link

mui-bot commented May 24, 2024

@aarongarciah aarongarciah added component: Typography The React component. package: material-ui Specific to @mui/material labels May 27, 2024
@aarongarciah aarongarciah self-assigned this May 27, 2024
@aarongarciah aarongarciah added the deprecation New deprecation message label May 27, 2024
@danilo-leal danilo-leal changed the title deprecate the <Typography paragraph> prop [material-ui][Typography] Deprecate the paragraph prop May 27, 2024
@zannager zannager requested a review from michaldudak May 27, 2024 14:24
@aarongarciah aarongarciah requested review from aarongarciah and removed request for michaldudak May 27, 2024 14:35
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Thanks @walston! I left a small comment. Apart from that, deprecating the paragraph prop in Typography requires some more work:

  1. Deprecate the corresponding class name: .MuiTypography-paragraph. You can add a @⁠deprecated JSDoc tag to where it's defined: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/Typography/typographyClasses.ts#L47-L48
  2. Run pnpm proptypes && pnpm docs:api and commit the generated changes. These commands copy the corresponding code comments from .d.ts to .js files and update the corresponding .json files that the docs will read when building the docs website.
  3. Document both deprecations (prop and class name) in the Migrating from deprecated APIs page (code). We can take inspiration from other deprecated props/class names documented on that very page. Remember to sort the newly added section alphabetically.
  4. Remove all usages of paragraph within the codebase. The prop is being used in around 20 files. Maybe replacing paragraph with component="p" gutterBottom in some cases is fine, but the spacing will certainly change and this will cause some visual regression tests to fail. We'll need to check case by case.

Remember to read the contributing guidelines and feel free to ping me if you need help.

packages/mui-material/src/Typography/Typography.d.ts Outdated Show resolved Hide resolved
@walston
Copy link
Contributor Author

walston commented May 28, 2024

thanks so much for the instructions @aarongarciah. Happy to make those changes later today.

@aarongarciah
Copy link
Member

Hey @walston! Did you have a chance to take a look? We want to land this on v6, which should be out in the next few weeks.

@walston
Copy link
Contributor Author

walston commented Jun 6, 2024

oh dang. I'd love that. Apologies, got caught up with a company onsite. I can spend some time working through these things next week. Top of my priorities list now!

@walston
Copy link
Contributor Author

walston commented Jun 6, 2024

@aarongarciah As I update the migration doc, it looks like there's an expectation there will be a codemod to handle this... Am I expected to write that codemod?

@aarongarciah
Copy link
Member

@walston in this case, I think a codemod doesn't make sense because there's no direct replacement for the paragraph behavior:

  1. Renders as a <p> element.
  2. Adds margin-bottom: 16px.

We could codemod number 1 with component="p", but we can't codemod number 2 because we can't possibly know what other styles are developers applying to the component instance and we might break their UIs. In this case, I think it makes sense to provide the instructions on how to migrate.

Even if it was possible, you could ask us to pick it up if you can't do it.

@arronhunt
Copy link
Contributor

@aarongarciah I'm working with @walston to get this over the finish line :)

We could codemod number 1 with component="p", but we can't codemod number 2 because we can't possibly know what other styles are developers applying to the component instance and we might break their UIs. In this case, I think it makes sense to provide the instructions on how to migrate.

For 1: I don't think we event need to add the component prop, since <Typography> renders as a <p> by default. Unless a developer overrides this setting it would probably result in a lot of redundant code. Is there a way to check for this?

For 2: Would we want to suggest adding mb={2} for Typography elements that need margin? I'm still confused about MUI's preferences between mb and sx props for margins.

@aarongarciah
Copy link
Member

Hey @arronhunt! Thanks for working on this.

From your comment I'm not sure if you're working on a codemod or not. Just in case, I think we shouldn't spend time in a codemod. Instead, we can focus on writing clear guidance on what changed in the Migrating from deprecated APIs page given we can't possibly know users' intent and where the styles are coming from.


For 1: I don't think we event need to add the component prop, since renders as a

by default. Unless a developer overrides this setting it would probably result in a lot of redundant code. Is there a way to check for this?

The tag being rendered by Typography depends on the variant and some other props. Typography renders a <p> in these scenarios:

  • variant="body1" (body1 is also used when no variant is passed)
  • variant="body2"
  • paragraph
  • component="p" or component={MyCustomComponent} (that renders a <p>)
  • variant="any_variant" paragraph
  • variant="any_variant" component="p"

It's fine to omit component="p" if no variant is passed or if the variant is already rendering a <p> (body1 and body2). See this demo for more info https://mui.com/material-ui/react-typography/#usage.

For 2: Would we want to suggest adding mb={2} for Typography elements that need margin? I'm still confused about MUI's preferences between mb and sx props for margins.

We'll remove system props (e.g. mb={2}) so it's recommended to use the sx prop. See this RFC and this in-progress PR if you want to know more.

@aarongarciah
Copy link
Member

Hey @arronhunt @walston! Let me know if you need any help.

@arronhunt
Copy link
Contributor

@aarongarciah I just pushed updates to the docs, using sx props to set bottom margin for paragraphs. I agree that a codemod isn't necessary and we can rely on the upgrade instructions. Would love your feedback!

@aarongarciah
Copy link
Member

@DiegoAndai I just updated the codemod to cater for mb, too. Ready for a final review.

@aarongarciah aarongarciah force-pushed the 42382-deprecate-paragraph-prop branch from e0cc8c7 to 78ffcca Compare July 22, 2024 18:16
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Nice 🚀

@aarongarciah aarongarciah merged commit 86f5173 into mui:next Jul 23, 2024
22 checks passed
@aarongarciah
Copy link
Member

Thanks @walston @arronhunt for working on this!

joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
Signed-off-by: Nathan Walston <walston@users.noreply.github.com>
Signed-off-by: Arron Hunt <arronhunt@users.noreply.github.com>
Co-authored-by: Aarón García Hervás <aaron.garcia.hervas@gmail.com>
Co-authored-by: Arron Hunt <arronjhunt@gmail.com>
Co-authored-by: Arron Hunt <arronhunt@users.noreply.github.com>
Co-authored-by: Aarón García Hervás <aaron@mui.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component. deprecation New deprecation message package: material-ui Specific to @mui/material v6.x
Projects
None yet
5 participants