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

[SAGE-497] Label - align to spec #584

Merged
merged 4 commits into from
Jul 19, 2021
Merged

Conversation

QuintonJason
Copy link
Contributor

@QuintonJason QuintonJason commented Jun 17, 2021

Closes SAGE-497
Dependent on SAGE-554

Description

  • align label to spec
    • update interactive border-radius
    • reduce label text font-weight

Screenshots

Before After
Screen Shot 2021-06-21 at 8 51 06 AM Screen Shot 2021-06-21 at 8 50 26 AM

Testing in sage-lib

Visit the label rails and react page

Testing in kajabi-products

  1. (LOW) Label component adjusted to latest specs. Label changes will occur as a result in kajabi-products but no adverse effects expected.

Related

@QuintonJason QuintonJason linked an issue Jun 17, 2021 that may be closed by this pull request
@QuintonJason QuintonJason self-assigned this Jun 17, 2021
@QuintonJason QuintonJason marked this pull request as ready for review June 21, 2021 14:01
@QuintonJason QuintonJason changed the title Label - align to spec [497] Label - align to spec Jun 21, 2021
@QuintonJason QuintonJason changed the title [497] Label - align to spec [SAGE-497] Label - align to spec Jun 21, 2021
Copy link
Member

@teenwolfblitzer teenwolfblitzer left a comment

Choose a reason for hiding this comment

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

Looking fantastic! Just to confirm, will the focus outline states be handled in a separate button/interaction PR?

Copy link
Member

@teenwolfblitzer teenwolfblitzer left a comment

Choose a reason for hiding this comment

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

👍🏼 LGTM!

Firefox seems to be rendering the alignment in its own way from Chrome/Safari, so the added transform/margin ends up pushing the button an additional 1px off the vertical line. I don't think it's a major concern, though:

Copy link
Contributor

@philschanely philschanely left a comment

Choose a reason for hiding this comment

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

Looks and works well. 🙌 One non-blocker is this older interactive—effect which I imagine will go away soon given your fabulous new interactive effects PR—does look a little odd with the new pill shape:

Screen Shot 2021-06-24 at 10 27 05 AM

Definitely an optional fix but wanted to raise awareness just in case it was of interest to fix now.

@QuintonJason QuintonJason force-pushed the SAGE-497_qj-label-spec-update branch from 8ce4b2d to 5cd8c80 Compare July 14, 2021 13:11
@QuintonJason QuintonJason requested review from philschanely and a team July 15, 2021 19:00
Copy link
Contributor

@philschanely philschanely left a comment

Choose a reason for hiding this comment

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

Looking good, @QuintonJason .

These may deserve a follow-up or two, but I noted the following:

  1. To match the specs closer it appears we need to revise the secondary button hover settings a bit:

    Spec:

    Screen Shot 2021-07-19 at 12 01 42 PM

    As built to date:
    Screen Shot 2021-07-19 at 12 10 55 PM

  2. There are also some aspects of how the colors and the mixture of sage btn within this component for the secondary button feel potentially leaky that might be nice revisit. See color settings in loop in _label.scss that override those in _button.scss. I wonder if we indeed need to use SageButton there or if the tradeoff is in our favor to use a custom styled HTML button instead?

Bottom line is nothing blocking this PR but some ideas about potential follow up. If you agree, I'm happy to file them in an issue for later. Thoughts?

@QuintonJason QuintonJason force-pushed the SAGE-497_qj-label-spec-update branch from 5cd8c80 to 8530c0c Compare July 19, 2021 17:20
@QuintonJason
Copy link
Contributor Author

Looking good, @QuintonJason .

These may deserve a follow-up or two, but I noted the following:

  1. To match the specs closer it appears we need to revise the secondary button hover settings a bit:
    Spec:
    Screen Shot 2021-07-19 at 12 01 42 PM
    As built to date:
    Screen Shot 2021-07-19 at 12 10 55 PM
  2. There are also some aspects of how the colors and the mixture of sage btn within this component for the secondary button feel potentially leaky that might be nice revisit. See color settings in loop in _label.scss that override those in _button.scss. I wonder if we indeed need to use SageButton there or if the tradeoff is in our favor to use a custom styled HTML button instead?

Bottom line is nothing blocking this PR but some ideas about potential follow up. If you agree, I'm happy to file them in an issue for later. Thoughts?

@philschanely Great callout! Made one commit to update hover colors, but we can revisit the need for the SageButton in a followup

@QuintonJason
Copy link
Contributor Author

QuintonJason commented Jul 19, 2021

@philschanely issue to investigate the inner button further #640

@QuintonJason QuintonJason merged commit 2fce833 into develop Jul 19, 2021
@QuintonJason QuintonJason deleted the SAGE-497_qj-label-spec-update branch July 19, 2021 18:40
@QuintonJason QuintonJason mentioned this pull request Jul 19, 2021
1 task
@FuturaExtraBold FuturaExtraBold mentioned this pull request Sep 14, 2021
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.

Label
3 participants