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

Open
wants to merge 23 commits into
base: next
Choose a base branch
from

Conversation

walston
Copy link

@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
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
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
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

@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

@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 added a commit with the codemod. I went with 16px instead of 2 for the marginBottom value.

One doubt: should we cater for the mb shortcut being present in the object passed to sx?

!!paragraphProp.value?.expression.value ||
paragraphProp.value?.expression.value === undefined;

if (!isParagraphPropTruthy) {
Copy link
Member

@aarongarciah aarongarciah Jul 12, 2024

Choose a reason for hiding this comment

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

The idea is that if paragraph={false} (or other falsy value) we don't add any margin, we just remove the paragraph prop.

@aarongarciah
Copy link
Member

I need to update the migration guide.

@aarongarciah
Copy link
Member

Ready for review.

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