-
Notifications
You must be signed in to change notification settings - Fork 333
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
Add max-width
to file upload
#2306
Conversation
@CharlotteDowns Is it intentional that we're making the file upload component always full width now, so it'll take up the full width of the container it's placed in? This does seem to match what we do for the text input component, but I know Ollie's original comment in the backlog seems to imply that's something we might not want to do for the file upload component. Example at default zoom on desktop the focus area for the component now looks like this: Whereas before it looked like this: |
@vanitabarrett I had the same concern. I'll do some further investigation work around the original width and the other implications of making the file-upload component |
Bear in mind the file upload input type is displayed very differently across different browsers and operating systems. I think we'd want to test this pretty thoroughly across all of our supported combinations to be sure. Ideally it'd be good to get before/after screenshots for all of them. Most of our other form inputs will grow to fill the available space, so I can see the argument for doing the same here. Aside from the way the focus state displays, it also means there can be quite a lot of invisible 'clickable area' in some browsers – which might also be an issue on mobile when scrolling? (Now it's been pointed out, I've no idea why the file upload in Chrome doesn't shrink to tightly wrap the button and filename label – I've no idea why it's the width it is.) |
0a5cd4f
to
7d38d94
Compare
Testing across browsers, before and afterWe've had a look into the before behaviour of the focused file upload component compared with the focused file upload component at On Chrome version 92.0.4515.159 without max-width the component requires right scrolling On Chrome version 92.0.4515.159 with max-width the component is contained within the width of the browser size More examplesExamples across the other browsers that we support can be found in this slide deck. We also compared the form elements page in the GOV.UK Frontend review app before and after to see how the behaviour compares to the other form field components we have in the Design System. TakeawaysThe change to add max-width seems to test well with 400% zoom across the browsers we support. IE8 seems to take content off the left of the screen which at the moment we are not sure why. |
@36degrees Updated the PR description with those screenshots now |
Looks good, thanks 🙂 Just needs a changelog entry I think? |
6cbd2cc
to
7508c5e
Compare
7508c5e
to
c528bd9
Compare
c528bd9
to
859d856
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.
👍🏻
max-width
to file upload
What we've changed
This PR adds a width attribute to the file-upload component.
Fixes #2007 .
Why we've changed it
When a user magnifies to 400% zoom the component does not resize to fit the viewport and horizontal scrolling appears.
Before / After
Chrome, MacOS
Safari, MacOS
Note: Safari Zoom doesn’t seem to go to as high a zoom level as Chrome so you don’t normally see the issue even on the ‘before’ when zooming to max. However, it's possible to replicate the issue by zooming to max and also making the Safari window narrower, as if on a tablet.
Firefox, MacOS
IE11
Note: ignore the floating browserstack menu!
IE10
Note: ignore the floating browserstack menu!
IE9
Note: ignore the floating browserstack menu!
IE8
Note: ignore the floating browserstack menu! Change doesn't seem to have any effect in IE8. Not sure what’s going on with the zooming here, IE8 zooming seems to take the content off the left side of the screen, but this happens in both versions so not a result of this change.