-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
Added btn--primary class to "Continue" button in New report page #4943
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4943 +/- ##
==========================================
- Coverage 82.41% 82.40% -0.01%
==========================================
Files 409 409
Lines 32042 32042
Branches 5109 5109
==========================================
- Hits 26406 26404 -2
- Misses 4123 4125 +2
Partials 1513 1513 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of Continue buttons in this template, but you've only updated one of them, was that deliberate?
I guess it looks like the difference was the "Continue" button was steps within the process, and then the "Submit" at the end is already btn--primary as the final actual-submission step. But I guess that doesn't really matter/make any difference, if you think it makes more sense for them all to be the same, okay with me.
I don't know how many things would need tidying up, I wouldn't want this to become a giant thing, but as always if it ends up making things simpler for the future, is good.
My thought was there is only one button visible at a time, so it didn't make much difference. With a "back" button right next to the "continue" one it would have more meaning to have a classic "primary" and "secondary" button approach.
I think I'm following the same approach we have been following over the last year or so, trying to standardised the branding process with new variables and reduce the need of overrides =) |
9fd9b96
to
ef44e42
Compare
d6fb707
to
0c830cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of tiny comments is all
@dracos Thanks for the feedback =) I just added a fixup for the comments/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :)
3914400
to
63c3bd3
Compare
@@ -36,7 +40,7 @@ $button-margin-left: 0 !default; | |||
font-weight: bold; | |||
font-family: inherit; | |||
cursor: pointer; | |||
border-radius: 4px; | |||
border-radius: $button-border-radius; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dracos just in case Cypress was failing some test, I noticed it because at some point we dropped the border-radius
mixin, but this PR was was still using it so I replaced it with border-radius
property instead.
Originated from mysociety/societyworks#4246 The current disabled state of the "continue" button is very similar to its default state. As a solution I think it's a good idea to take this opportunity and give it the primary button styling so it matches the cobrands branding. Also I have updated the button-variant mixin, so the disabled state has a more evident disabled state.
63c3bd3
to
bc0538e
Compare
Fixes: https://github.com/mysociety/societyworks/issues/4246
With this PR "Continue" button will have same styling as
btn--primary
, to match the branding of each cobrand, the disabled state has also been updated so it has a more evident "disabled" state.NOTE: To fix the original issue, including
btn-primary
is more of a bonus, but we could also get away with keeping what we currently have and add transparentize effect that passes the contrast colour to the disabled state. However this could be an opportunity to make a standard across all cobrands the use of the mixinbutton-variant
forbtn--primary
. As an extra I think we could also include an extra variable in the mixin to control theborder-radius
property. We would be covering more topics, so happy to open a new issue to update the "button-variant" if you think that would be better.Preview
PR4943.pdf
Master Preview(What we currently have)
Master.pdf
Default:
Disabled
[Skip changelog]