nhsuk / nhsuk-frontend

NHS.UK frontend contains the code you need to start building user interfaces for NHS websites and services.
https://nhsuk.github.io/nhsuk-frontend/
MIT License
620 stars 107 forks source link

Update height and set minimum width of Selects #987

Closed frankieroberto closed 2 months ago

frankieroberto commented 2 months ago

This fixes a minor bug with the select component in Safari, which is that the min-height property is ignored when the default native appearance is used, resulting in a select which is only 30px high. (Other browsers such as Chrome and Firefox seem to apply the minimum height.)

This restores the select to its previous fixed height of 40px. As the select will never wrap onto 2 or more lines (instead the text gets truncated), having a fixed height should be fine.

Additionally, this also introduces a minimum width to the select component (this property does work in all browsers), in case the options in the select are very short. In these cases, having a wider select means for a bigger target area, and is closer to what users expect. I’ve followed the GOV.UK Design System in setting the minimum width to match the width-10 variant of the input component, which works out as 223px (see their definition of this).

Screenshots (Safari)

Before After
Screenshot 2024-07-18 at 08 55 11 Screenshot 2024-07-18 at 08 55 28
Screenshot 2024-07-18 at 09 00 03 Screenshot 2024-07-18 at 08 59 47

Checklist

anandamaryon1 commented 2 months ago

@frankieroberto Looked into this one to try and fix your safari issue but also fix the initial issue that introduced this bug: https://github.com/nhsuk/nhsuk-frontend/issues/921

Swapped to set the height in rems, seems to work. Sadly can't take credit for the idea – it's how GOV have sized theirs.

(also refactored your comments to follow the style guide)

frankieroberto commented 2 months ago

@anandamaryon1 oh nice! I didn't use rems as we didn't seem to use rems elsewhere, but it will also make it scale with the text size. 🙌

I think the Safari bug is more down to setting min-height (which seems to be ignored) rather than the unit type, but not 100% sure.