gsoft-inc / sg-orbit

The design system for ShareGate web apps.
https://orbit.sharegate.design
Apache License 2.0
101 stars 37 forks source link

🐛 `rem` units not resizeable by user agent unless `postcss-pxtorem` is used. #1072

Open tjosepo opened 1 year ago

tjosepo commented 1 year ago

Describe the bug

By default, Orbit sets the body font-size to 16px, which overrides the user's preferred font-size.

Unless a package like postcss-pxtorem is used to convert the 16px into 1rem, the user is unable to change the page's font-size.

Steps to reproduce

Go to https://orbit.sharegate.design/ and resize the font size from your browser's settings. There should not be any visual changes.

Expected results

Font size should be resized to the user's desired font size without requiring postcss-pxtorem.

The body font-size should be 1rem, not 16px.

patricklafrance commented 1 year ago

Hey @tjosepo!

What do you mean by allowing the user to override the font-size?

Are you referring to zooming the page or it’s something else?

tjosepo commented 1 year ago

Most browsers and operating systems have the option to let users increase the size of fonts without zooming on a page.

On Chrome, this can be changed in Settings > Appearance> Change font size

However, this feature doesn't work if a non-scalable unit (like px) is used for font-size on the body element.

patricklafrance commented 1 year ago

Got it, thanks for the clarification.

I might be wrong but I think that the issue with this change is that the scale won't work anymore.

It's been done this way so the system can assume that from there 1em equal 16px instead of the default browser value. Might not be the best way to do it thought if it's a value that the user can customized.

What do you think @fraincs ?

fraincs commented 1 year ago

@patricklafrance resizing your whole browser default font size it a feature not that used, if a user does it it is because he has accessibility issues, it's a matter of how attached are we to everything being the size we want vs if we want to accomodate the users knowing that they are probably aware that some website would not look their "best".

It has to be tested, but since we test zooming in and out to an extent, I am confident that changing the root font-size would not break our components, only make them look bigger or smaller, the same with the zoom in / out.

Indeed supporting this would need to go hand in hand with the apps supporting this too/

patricklafrance commented 1 year ago

@fraincs I wonder how that would affect the style system? As right now all the classes expect that the default base font-size is 16px. Would it break anything?

fraincs commented 1 year ago

@fraincs I wonder how that would affect the style system? As right now all the classes expect that the default base font-size is 16px. Would it break anything?

same that we expect the user not to zoom and we are ok since we are using rems, I will validate but I think it'll be ok.

fraincs commented 1 year ago

@patricklafrance

image

Here's an example of what would happen if we let the user change the default font size. A whole QA should be done before releasing this although I see it as a progressive enchantment. Everything will still works as all browsers are set to 16 by default but we would allow a user to change it.

Github is enforcing a 16px font size and does not react to browser changing it's font-size.

patricklafrance commented 1 year ago

Also tried it.

Personally I think it works pretty well with Orbit.

Trying it also reveal a few issue with the document / Orbit.

For example, code block are using a fixed font size:

image

Inputs default width also seems problematic:

image

image

Checkmark size doesn't seems to follow:

image

Not sure, but there might also be an issue the Listbox scrollbar logic:

image

I also wonder if modal size should be adapted?:

image

I think there's an issue with the tile checkmark :)

image

patricklafrance commented 1 year ago

IMO we should do it... Having a font-size adapted to a user condition is pretty basic accessibility support.

Here's what I think should be done to support it:

  1. Fix the issues I reported in my previous message
  2. QA for other issues
  3. Find a way to test this in Chromatic
  4. See how that would affect the UX if Orbit support custom font-size but the custom application code doesn't
fraincs commented 1 year ago

IMO we should do it... Having a font-size adapted to our condition is pretty basic accessibility support.

Here's what I think should be done to support it:

  1. Fix the issues I reported in my previous message
  2. QA for other issues
  3. Find a way to test this in Chromatic
  4. See how that would affect the UX if Orbit support custom font-size but the custom application code doesn't

Sounds like a plan. As for 4, we would need a buy in from all front-ends. Right now nothing moves in theory where now orbit components would react but not part of the apps, which might be worst that what we have now.

fraincs commented 1 year ago

Not sure, but there might also be an issue the Listbox scrollbar logic: image Can you elaborate? I haven't noticed anything on my end.

patricklafrance commented 1 year ago

Not sure, but there might also be an issue the Listbox scrollbar logic: image Can you elaborate? I haven't noticed anything on my end.

If I remember correctly there’s an algorithm calculating the initial number of items to show. The algorithm goal is to minimize times when a vertical scrollbar is displayed. I don’t think there should be an initial scrollbar here.

fraincs commented 1 year ago

Thing is that I don't have one hence my question where do you see this?

patricklafrance commented 1 year ago

Thing is that I don't have one hence my question where do you see this?

Listbox docs