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

Remove toolkit button sass #1016

Merged
merged 2 commits into from
Apr 1, 2019
Merged

Remove toolkit button sass #1016

merged 2 commits into from
Apr 1, 2019

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Mar 28, 2019

Removes the last dependency on govuk_frontend_toolkit, for button styles. Most of the buttons in finder-frontend had already been converted to use the button component already.

This change involves a visual change on the subscription page. This button is now using the button component:

Before

Screen Shot 2019-03-28 at 14 12 41

After

Screen Shot 2019-03-28 at 14 12 51

This change unfortunately required a custom wrapper on the text above in order to provide margin bottom. Hopefully this will be made obsolete at some point if the govspeak component is given margin bottom.

Note: in replacing this button I noticed that the submit_tag rails command adds a data attribute of data-disable-with to the outputted HTML. I don't know if this is important but to be safe I've configured the button component to include this attribute.

Example links:

I don't think there's any other buttons, but happy to be proved wrong.

@benthorner benthorner temporarily deployed to finder-frontend-pr-1016 March 28, 2019 14:20 Inactive
@benthorner benthorner temporarily deployed to finder-frontend-pr-1016 March 28, 2019 15:33 Inactive
Copy link
Contributor

@sihugh sihugh left a comment

Choose a reason for hiding this comment

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

Looks great apart from the unnecessary disable-with bit. 👍

I concur that I think the buttons you found are the only ones.

app/views/email_alert_subscriptions/new.html.erb Outdated Show resolved Hide resolved
- margin bottom is already defined for this component in its own CSS
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.

4 participants