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

UX: Make input sizing consistent across all browsers #26159

Merged
merged 2 commits into from Mar 20, 2024

Conversation

davidtaylorhq
Copy link
Member

@davidtaylorhq davidtaylorhq commented Mar 13, 2024

Previously we had an iOS-specific sizing rule which would increase inputs to 1.07em, which would bring them over the 16px 'zoom on focus' threshold in some (but technically, not all) situations.

This commit does two things:

  1. Updates the sizing rule from 1.07em to max(1em, 16px). Essentially: use the cascaded font size, unless it is smaller than 16px

  2. Applies that sizing rule on all platforms. This will make Discourse design/theming more consistent across different devices

Comment on lines 118 to 119
padding-top: $vpad * 0.8; // TODO... what's this for?
padding-bottom: $vpad * 0.8; // TODO... what's this for?
Copy link
Member Author

Choose a reason for hiding this comment

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

@pmusaraj do you remember the reasoning on this? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Some context from the PR:

  • the values I chose for vertical and horizontal padding are the closest I could get to the current sizing in core. I’m not married to them, my goal is to get to consistent sizing with as little visual change as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bit of context in the original PR #14226

* 0.8 specifically was probably to make sure the inputs kept the same height as before on iOS to compensate for the font size bump. Now that we're thinking of standardising, would be good to pick one or the other... likely best without it.

A couple of places to check before/after differences on various devices:

  • the composer
  • /styleguide/atoms/input-fields

Copy link
Member Author

Choose a reason for hiding this comment

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

to make sure the inputs kept the same height as before on iOS to compensate for the font size bump

Ah I see, that does make sense. Although I guess since then, other things have changed. Because the overall input size on iOS is definitely larger than on other devices.

Now that we're thinking of standardising, would be good to pick one or the other... likely best without it.

Agreed 👍. Dropping it

A couple of places to check before/after differences on various devices:

Thanks for the tip! Both are looking good to me following this change 🤞

-webkit-tap-highlight-color: transparent;
}
textarea {
background-color: var(--secondary); // TODO: why was this iOS-only
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to test with/without this on a few browsers to work out why 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might have been a mistake? see https://github.com/discourse/discourse/pull/7480/files#diff-ba31ce59ea78b5348c6186b59600316cd0922365355d867c63b7de632e1920ca

it was previously applied on all mobile browsers, and that PR didn't justify the change

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, dropping it

textarea {
background-color: var(--secondary); // TODO: why was this iOS-only
font-size: var(--font-size-ios-input);
-webkit-tap-highlight-color: transparent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Per MDN:

-webkit-tap-highlight-color is a non-standard CSS property that sets the color of the highlight that appears over a link while it's being tapped.

so… does it even do anything for textarea elements? 🤔 or input elements like the style below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird! I'll remove it

Previously we had an iOS-specific sizing rule which would increase inputs to `1.07em`, which would bring them over the 16px 'zoom on focus' threshold in some (but technically, not all) situations.

This commit does two things:

1. Updates the sizing rule from `1.07em` to `max(1em, 16px)`. Essentially: use the cascaded font size, unless it is smaller than 16px

2. Applies that sizing rule on all platforms. This will make Discourse design/theming more consistent across different devices
@davidtaylorhq davidtaylorhq marked this pull request as ready for review March 20, 2024 15:40
Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

Alright, let's give this a shot!

@davidtaylorhq davidtaylorhq merged commit d0d4a36 into main Mar 20, 2024
16 checks passed
@davidtaylorhq davidtaylorhq deleted the ux-input-sizing branch March 20, 2024 16:23
davidtaylorhq added a commit that referenced this pull request Mar 21, 2024
This reverts commit d0d4a36. This causes issues for people that have specified explicit font sizes in their browser - reverting while we investigate. https://meta.discourse.org/t/300374
davidtaylorhq added a commit that referenced this pull request Mar 21, 2024
#26295)

This reverts commit d0d4a36. This causes issues for people that have specified explicit font sizes in their browser - reverting while we investigate. https://meta.discourse.org/t/300374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants