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

Changes_for_bugfix-872_chore-869-870_task #883

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

adhiyan-tangoe
Copy link
Contributor

@adhiyan-tangoe adhiyan-tangoe commented Sep 27, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added
  • Docs have been added or updated

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Style Update (CSS)
  • Other... Please describe: Chore task

What is the current behavior?

Secondary button, secondary split button and secondary button group were missing its border when in a disabled state.
Resolves 872
Secondary button, secondary split button and secondary button group will have the border : 1px solid #d3d3d3

Form hints does not match the UX approved design
Resolves 870
Form text will be italicized and with size 12px

Error form hints does not match the UX approved design
Resolves 869
Error status size changed from 8px to 9px and use Horizon (#DB3939) for color and hint will be italicized

Does this PR introduce a breaking change?

  • Yes
  • No

@Saravanan-Tangoe Saravanan-Tangoe added bug Something isn't working chore General chore or maintenance task UX Review Waiting on review from UX labels Sep 27, 2022
@Saravanan-Tangoe Saravanan-Tangoe changed the title Changes_bugfix_chore_task Changes_for_bugfix-872_chore-869-870_task Sep 27, 2022
@Saravanan-Tangoe Saravanan-Tangoe self-assigned this Sep 27, 2022
@@ -33,7 +34,9 @@

&__status {
display: block;
font-size: .5rem;
font-size: 0.5625rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Part of me feels like having a font-size go 4 digits out is a little overkill... if we need to make it bigger could we make it .56 or .57? Unless there was a specific request/reason for making it .5625, I think 2 digits is more maintainable.
Just realized there is a .5625rem value up above too...

Copy link
Contributor

Choose a reason for hiding this comment

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

i see a requirement to change the size from 8px to 9px, do you think is that okay to keep as .56 which is (8.96px)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Saravanan-Tangoe oh I see. If that's the case then I think that's fine, even though it seems weird haha.

@jiaweicai92 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed it to 9px instead of 0.5625rem

Copy link
Contributor

Choose a reason for hiding this comment

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

@swimtech20 @Saravanan-Tangoe The change makes sense. 👍 Great job guys

Copy link
Contributor

@jiaweicai92 jiaweicai92 left a comment

Choose a reason for hiding this comment

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

Looks good to me💯

Copy link
Collaborator

@swimtech20 swimtech20 left a comment

Choose a reason for hiding this comment

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

Looks good!

@stolve
Copy link

stolve commented Jan 27, 2023

@kpmurphh and myself have no way to provide approval/feedback for this issue. We do not have an instance of goponents on our systems in order to look at the visual fix and we are not in a position to give feedback on coding approaches. Please attach a screenshot of the update or block time on one of our calendars for a review session.

@RonMichaud RonMichaud merged commit f201378 into dev Mar 30, 2023
@RonMichaud RonMichaud deleted the BUGFIX-872_and_CHORE-869-870 branch March 30, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore General chore or maintenance task UX Review Waiting on review from UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Secondary Button Disabled State [Chore] General Form Hint [Chore] Error Form Hints
6 participants