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.

Sign up for GitHub

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

Jump to bottom

Migrate 'Note' component away from makeStyles + cleanup #630

Merged
merged 7 commits into from
Apr 11, 2023
Merged

Migrate 'Note' component away from makeStyles + cleanup #630

merged 7 commits into from
Apr 11, 2023

Conversation

jantolg
Copy link
Contributor

@jantolg jantolg commented Apr 10, 2023

Description

Shortcut: 297037

Type of change

  • Refactor

Acceptance

Same previous styles, but with MUI5

Basic checklist

  • Good PR name
  • Shortcut link
  • Changelog entry
  • Just one issue per PR
  • GitHub labels
  • Proper status & reviewers
  • Tests
  • Documentation

@jantolg jantolg requested a review from vmilan April 10, 2023 11:41
@coveralls
Copy link
Collaborator

coveralls commented Apr 10, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4664601802

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 71.338%
Totals Coverage Status
Change from base Build 4658544875: -0.01%
Covered Lines: 1948
Relevant Lines: 2512
💛 - Coveralls

vmilan
vmilan reviewed Apr 10, 2023
View reviewed changes
packages/react-ui/src/widgets/legend/Note.js
@@ -19,9 +16,9 @@ export default function Note({ children }) {
return (
<Box mt={1} data-testid='note-legend'>
<Typography variant='caption'>Note:</Typography>{' '}
<Typography className={classes.note} variant='caption'>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer necessary, as the caption variant has a regular weight by default.

You can use just <Typography variant='caption' /> and remove the customization.

vmilan
vmilan approved these changes Apr 10, 2023
View reviewed changes
Copy link
Contributor

@vmilan vmilan left a comment

Choose a reason for hiding this comment

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

👍

@jantolg jantolg changed the title Migrate Note componente to MUI5 Migrate Note component to MUI5 Apr 10, 2023
@VictorVelarde
Copy link
Contributor

VictorVelarde commented Apr 10, 2023 •
edited
Loading

@jantolg I would suggest to:

  • name all PRs like Migrate 'XXXX' component away from makeStyles, not MUI5, as makeStyles is compatible with MUI-5 (but not with React 18). I have changed this one, as an example
  • add more details if PR includes anything else (like '...and refactor / cleanup')
  • add an entry in the changelog per PR, in the not released section
  • land PR after 2 approvals, mine & Vero's, at least initially
  • use always squashed merge, using the PR title for the commit message
  • always verify component result in Storybook, have you @jantolg? You can easily compare the new 'local' and play with current version 'stable for v2.0'. The second one is currently at https://ds-storybook-xp-carto-carto-frontend.vercel.app/, but it will be very soon published into the official storybook, at https://storybook-react.carto.com/

@VictorVelarde VictorVelarde self-requested a review April 10, 2023 15:03
@VictorVelarde VictorVelarde changed the title Migrate Note component to MUI5 Migrate 'Note' component away from makeStyles + cleanup Apr 10, 2023
VictorVelarde
VictorVelarde approved these changes Apr 10, 2023
View reviewed changes
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM, thx a lot!

@jantolg jantolg merged commit f8196c4 into master Apr 11, 2023
@jantolg jantolg deleted the feat/migrate-mui5-note-component branch April 11, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmilan vmilan vmilan approved these changes

@VictorVelarde VictorVelarde VictorVelarde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jantolg @coveralls @VictorVelarde @vmilan