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

Upgrade components gem #1307

Merged
merged 5 commits into from
Aug 21, 2019
Merged

Upgrade components gem #1307

merged 5 commits into from
Aug 21, 2019

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Aug 15, 2019

Updates the components gem in finder-frontend to version 18, which includes GOVUK Frontend V3, which includes breaking changes. This PR fixes those breaking changes.

Visual changes

Changes to the option select component due to focus change styles. Before (focussed):

Screen Shot 2019-08-15 at 15 01 28

After (focussed):

Screen Shot 2019-08-16 at 11 38 09

(thanks to @injms for the style suggestion!)

The expander component has similar focus style changes, but as it's not in use anywhere I'm less worried about it.

Shouldn't be any other visual changes, I've looked through a load of finders and they all look like they used to.


Search page examples to sanity check:

Other finders

@bevanloon bevanloon had a problem deploying to finder-frontend-pr-1307 August 15, 2019 13:58 Failure
@andysellick andysellick force-pushed the upgrade-component-gem branch from d836678 to caee7c0 Compare August 16, 2019 08:45
@bevanloon bevanloon had a problem deploying to finder-frontend-pr-1307 August 16, 2019 08:45 Failure
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1307 August 16, 2019 08:56 Inactive
@andysellick andysellick force-pushed the upgrade-component-gem branch from 9d447ca to 9884632 Compare August 16, 2019 08:57
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1307 August 16, 2019 08:57 Inactive
@andysellick andysellick changed the title [DO NOT MERGE] [WIP] Upgrade component gem Upgrade component gem Aug 16, 2019
@andysellick andysellick changed the title Upgrade component gem Upgrade components gem Aug 16, 2019
@andysellick andysellick force-pushed the upgrade-component-gem branch from 9884632 to 73fb0e3 Compare August 16, 2019 09:19
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1307 August 16, 2019 09:20 Inactive
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

I added two suggestions otherwise good to go.

app/assets/stylesheets/application.scss Outdated Show resolved Hide resolved
@andysellick andysellick force-pushed the upgrade-component-gem branch from 73fb0e3 to 31e3758 Compare August 16, 2019 09:53
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1307 August 16, 2019 09:53 Inactive
@injms
Copy link
Contributor

injms commented Aug 16, 2019

Not a designer, so feel free to disregard - I think that the focus state where the whole box is outlined in yellow ties together better than just having the focus state on the button.

Maybe something like

Screen Shot 2019-08-16 at 11 23 26

could reinforce the focus of the whole component?

@andysellick
Copy link
Contributor Author

@injms niiiiiiiice

@vanitabarrett
Copy link
Contributor

The focus state between document list title and description is very close - is this a known issue with the component since the upgrade? I know there were a few issues with focus state and line height.

Screen Shot 2019-08-16 at 12 13 24

@andysellick
Copy link
Contributor Author

@vanitabarrett yes, that's tied into that whole issue. @injms made some changes to related links but there are many other components affected. I'll take a look at the document list component and maybe put a checklist on that issue so we can keep track of what's been fixed.

@andysellick andysellick changed the title Upgrade components gem [DO NOT MERGE] Upgrade components gem Aug 17, 2019
@andysellick
Copy link
Contributor Author

@alex-ju @vanitabarrett @injms @DilwoarH @maxgds just realised I can't merge this until we've done at least government-frontend, frontend, and collections - otherwise the site will have inconsistent focus states.

@andysellick andysellick changed the title [DO NOT MERGE] Upgrade components gem Upgrade components gem Aug 21, 2019
@andysellick andysellick merged commit 4de9588 into master Aug 21, 2019
@andysellick andysellick deleted the upgrade-component-gem branch August 21, 2019 10:27
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.

5 participants