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

Migrate 'LegendProportion' component from makeStyles to styled-components + cleanup #635

Merged

Conversation

jantolg
Copy link
Contributor

@jantolg jantolg commented Apr 11, 2023

Description

This PR includes a refactorization of LegendProportion component, all classes from makestyles are been converted to styled components

Shortcut: 297037

Type of change

  • Refactor

Acceptance

The code resultant must return same styles component visualization as the same before changes

  1. Overview a general visualization from v1.x to v2.x

Basic checklist

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

@jantolg jantolg requested review from VictorVelarde and vmilan April 11, 2023 16:53
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.

Missing changelog note and there is a visual difference between your branch and master, please take a look.

Screenshot 2023-04-12 at 15 00 37

Comment on lines 37 to 40
<Circle index={0} component='span'></Circle>
<Circle index={1} component='span'></Circle>
<Circle index={2} component='span'></Circle>
<Circle index={3} component='span'></Circle>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe would be simpler just to do something like this:

        {[...Array(4)].map((circle, index) => (
          <Circle index={index} component='span'></Circle>
        ))}

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@@ -48,7 +48,7 @@ function LegendProportion({ legend }) {
spacing={1}
>
{error ? (
<Grid item className={classes.errorContainer}>
<Grid item xs maxWidth={240}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need xshere? or is it a leftover?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Comment on lines 9 to 16
const sizes = {
0: 96,
1: 72,
2: 48,
3: 24
};
const width = `${sizes[index]}px`;
const height = `${sizes[index]}px`;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use theme.spacing() also here.

Take a look at this component to see an example: packages/react-ui/src/components/molecules/Avatar.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@aaranadev
Copy link
Contributor

aaranadev commented Apr 13, 2023

I've fixed the visual difference

Screenshot 2023-04-13 at 9 44 28 AM

@coveralls
Copy link
Collaborator

coveralls commented Apr 13, 2023

Pull Request Test Coverage Report for Build 4690753089

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 71.345%

Totals Coverage Status
Change from base Build 4690716869: 0.0%
Covered Lines: 1949
Relevant Lines: 2513

💛 - Coveralls

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.

UI LGTM, just adds a note in the changelog please, and wait for @VictorVelarde review before merging.

Thanks!!

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, please changelog and we're free to land, thx!

@VictorVelarde VictorVelarde force-pushed the feat/migrate-styled-component-legend-proportion-component branch from 10411ba to 9ca557a Compare April 28, 2023 16:44
@VictorVelarde VictorVelarde merged commit befd65c into master Apr 28, 2023
@VictorVelarde VictorVelarde deleted the feat/migrate-styled-component-legend-proportion-component branch April 28, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants