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

HPC-9742 Modify button component style #465

Merged
merged 3 commits into from
Sep 11, 2024
Merged

HPC-9742 Modify button component style #465

merged 3 commits into from
Sep 11, 2024

Conversation

Onitoxan
Copy link
Contributor

Nature of PR

  • Bug-fix
  • Hot-fix
  • Feature
  • Testing
  • Rewrite
  • CI
  • Package update

Description

For more information read info in ticket HPC-9742. This PR includes code for modifying and adding new styles to our existing button for it to have more cohesion with how we are styling our components.

How to test changes

Verify that the modified styles show up as expected.

TODO:

No todo

@Onitoxan Onitoxan added the ready for review All comments have been addressed, and the Pull Request is ready for review label Sep 11, 2024
@Onitoxan Onitoxan requested a review from a team as a code owner September 11, 2024 07:18
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

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

Checks have passed and this pull request is ready for manual review

Copy link
Contributor

@Pl217 Pl217 left a comment

Choose a reason for hiding this comment

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

One minor thing I'll address myself and then merge this

text-decoration: none;
background-color: ${(p) => p.theme.colors.pallete.gray.light};
color: #fff;
}
&.${COLOR_CLS.neutral_light} {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks little out of place. You have added this definition where empty line was, which makes git report as single line was removed, while entire PR adds new code.

There is a certain order established in COLOR_CLS and it goes primary, primary_light, secondary, secondary_light, neutral, neutral_light, so I'd respect this order when using the variables and place neutral_light after secondary_light.

Not very important, but contributes to better readability.

@Pl217 Pl217 added needs minor changes There are review or issue comments to address and removed ready for review All comments have been addressed, and the Pull Request is ready for review labels Sep 11, 2024
We use MUI library to use various component,
in this page: https://mui.com/system/getting-started/the-sx-prop/#borders

> "The borderRadius property multiplies the value it
> receives by the theme.shape.borderRadius value
> (the default for this value is 4px)."

So to mantain some uniformity, we will use
the same border-radius in our button
@Pl217 Pl217 added ready for merge Review and testing is complete. It is ready for merging as soon as CI has finished. and removed needs minor changes There are review or issue comments to address labels Sep 11, 2024
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

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

Checks have passed and this pull request is ready for manual review

@Pl217 Pl217 merged commit 73db0c0 into develop Sep 11, 2024
3 checks passed
@Pl217 Pl217 deleted the HPC-9742 branch September 11, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge Review and testing is complete. It is ready for merging as soon as CI has finished.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants