-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fix header with product name focus and hover state length #2549
Conversation
@owenatgov Nice work on picking this up 👍 In the 'after' screenshot, I can see the bottom of the GOV.UK text gets cut off a little bit (more noticeable on the 'G' and dot), which probably isn't ideal. I wonder if there's anything we can do to stop that? |
@vanitabarrett Good spot, how strange! It looks like this is coming from the |
a39df1c
to
692c75c
Compare
@vanitabarrett I've found a solution that involved amending the |
692c75c
to
6366f9a
Compare
6366f9a
to
5f6aa35
Compare
Hm, I'm not 100% sure about this change. I think the change from Example: Before (234px wide)After (234px wide)Unfortunately I don't really have any other suggestions... I am wondering if this work would get any easier if we solved #1501, but maybe not. |
@vanitabarrett Bugger. I think you're right that the root of this is elsewhere. I'm pretty sure that issue is linked to this so I'll investigate if there's a way to solve both of these. |
5f6aa35
to
f648d2d
Compare
@vanitabarrett After assessing this a bit more, I reckon #1502 is going to take a more thorough investigation. For now I've found a quick fix that solves the small screen problem by using a media query to control the |
@owenatgov Thanks for digging into it a bit! I agree, it might need a more thorough investigation. I quite like this quick fix, nice work 👍 I'm wondering if the box-shadow change should also be inside the media query, as we're essentially not changing the focus state at all in lower breakpoints? |
f648d2d
to
e629503
Compare
@vanitabarrett 100%. Applied! |
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.
As mentioned in Slack, this just needs a Changelog entry and then it should be ready to go
@owenatgov Would you also mind adding the linked issue to the [next] milestone (indicating it will be release in the next version of GOV.UK Frontend)? |
e629503
to
3e4cb11
Compare
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
What
Changes the desktop display properly of the header homepage link from
inline-block
toinline
.Why
Addressing part of #2181
You can test this by starting a local review app, going to the header component and looking at the focus and hover states of "header with product name".
Visual changes
Focus state
Before:
After:
Hover state
Before:
After: