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

Define size of inputs etc in pixels rather than em #491

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Feb 1, 2018

This PR removes the calls to the em function from Frontend, so that all measurements
are defined in pixels.

The em conversions were originally added in an attempt
to make form elements line up. However, they cause problems when you try to use
Frontend in an application that defines font sizes, especially when those font
sizes are nested.

This also removes the em() call from govuk-c-details as the above problems
apply to this component too. For now, govuk-c-table still calls em - I'll
find out from Anika if there's any particular reason tables in Elements used
them.

Note: I will review the visuals with @dashouse, there might be things we need to adjust now.

As part of https://trello.com/c/WpPKyqxl/493-5-define-size-of-inputs-etc-in-pixels-rather-than-em

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-491 February 1, 2018 11:40 Inactive
@hannalaakso hannalaakso removed the request for review from kr8n3r February 1, 2018 11:40
Copy link

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

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

checkbox label: before still calls em()

@NickColley
Copy link
Contributor

How do we want these components to look on small screens?

I've done a manual visual diff before and after

before-and-after

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-491 February 1, 2018 12:03 Inactive
@hannalaakso
Copy link
Member Author

hannalaakso commented Feb 1, 2018

Thanks @igloosi that's now fixed.

Thanks @NickColley 👍 I've requested @dashouse to review the visuals, there might be things we want to adjust. I just added that to the description for clarity.

@hannalaakso hannalaakso changed the title Define size of inputs etc in pixels rather than em WIP: Define size of inputs etc in pixels rather than em Feb 1, 2018
@NickColley
Copy link
Contributor

@hannalaakso my gif is before you made your update, I'll update it now

@NickColley
Copy link
Contributor

before-and-after

Remove the calls to the `em` function from Frontend, so that all measurements
are defined in pixels. The `em` conversions were originally added in an attempt
to make form elements line up. However, they cause problems when you try to use
Frontend in an application that defines font sizes, especially when those font
sizes are nested.

This also removes the em() call from `govuk-c-details` as the above problems
apply to this component too. For now, `govuk-c-table` still calls `em` - I'll
find out from Anika if there's any particular reason tables in Elements used
them.
@dashouse
Copy link

dashouse commented Feb 1, 2018

Neither of @NickColley's GIFs look like what I'm seeing? I see them as perfectly lined up where as your screenshot still has a slight overlap?

Intention with these was to retain the physical size of the input at all breakpoints, even though the font-size gets smaller within.

@NickColley
Copy link
Contributor

@dashouse I'm on Windows at the moment, so might be fine on OSX

@hannalaakso
Copy link
Member Author

@NickColley What Windows OX/browser/browser version are you using?

@NickColley
Copy link
Contributor

NickColley commented Feb 1, 2018

@hannalaakso do you want me to raise an issue so this isnt blocked?

Edit:
Windows 10 Home
Chrome Version 63.0.3239.132
HiDPI screen

@hannalaakso
Copy link
Member Author

Thanks @NickColley that would be good. I'm not able to replicate this on Browserstack but happy for it to go in the backlog and I can pick it up and have another look in the accessibility lab when I'm in the office.

@hannalaakso hannalaakso changed the title WIP: Define size of inputs etc in pixels rather than em Define size of inputs etc in pixels rather than em Feb 2, 2018
@hannalaakso hannalaakso merged commit baf0bd9 into master Feb 8, 2018
@hannalaakso hannalaakso deleted the remove-em-from-inputs branch February 8, 2018 09:43
@36degrees
Copy link
Contributor

@hannalaakso Can you make sure we add something to the changelog for this? 🙂

@hannalaakso hannalaakso mentioned this pull request Feb 9, 2018
@hannalaakso
Copy link
Member Author

Done: #520 Thanks for the heads up @36degrees!

@alex-ju alex-ju mentioned this pull request Feb 20, 2018
hannalaakso added a commit that referenced this pull request Jun 28, 2018
This mirrors the changes made in
#491
hannalaakso added a commit that referenced this pull request Jul 6, 2018
This mirrors the changes made in
#491
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.

6 participants