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

toolbar using REM can cause a problem #20099

Open
pauldambra opened this issue Feb 1, 2024 · 7 comments
Open

toolbar using REM can cause a problem #20099

pauldambra opened this issue Feb 1, 2024 · 7 comments
Labels
bug Something isn't working right feature/toolbar Feature Tag: Toolbar

Comments

@pauldambra
Copy link
Member

A customer reported they have font-size set to 1px on their site to make rem calculations easier in their CSS https://posthoghelp.zendesk.com/agent/tickets/9837

Since we use rem in posthog, and toolbar shares styling with posthog, toolbar picks up the font-size from html...

On that customers site that meant the toolbar was 16x smaller than normal

We need to either

  • override the font-size or some other magic so we can carry on using rem
  • not share css between posthog and toolbar (probably not possible)
  • not use rem in posthog so toolbar doesn't either (probably not desirable)
@pauldambra pauldambra added bug Something isn't working right feature/toolbar Feature Tag: Toolbar labels Feb 1, 2024
@daibhin
Copy link
Contributor

daibhin commented Feb 2, 2024

I spent a lot of time looking into this today and I'm not sure there's anything we want to do here. What I'd really love is our own custom CSS unit e.g. phrem that we could specify as a variable e.g.

// in PostHog web app
--phrem: 1rem;

// in Toolbar
--phrem: min(max(8px, 1rem), 24px) stops toolbar getting too big or small

This might come to CSS in the future but the issue is stagnating for now.

Because shadow doms inherit colors & font sizes we have no way of overriding the rem values. I would argue changing this to hard code the toolbar to a specific size (e.g. 16px) would cause another customer to complain that we are not respecting their custom rem values. Plus it would mean that we would need to override every instance of a rem value in shared code and keep that up to date or live with UI inconsistencies (arguably the worst compromise of all)

Have we ever considered inserting the toolbar as an iframe rather than a shadow dom? I don't know how possible that is if it needs to interact with elements on the page but it would isolate the styles

@mariusandra
Copy link
Collaborator

Sadly we don't have access to the contents of the iframe in this case.

Could we just switch all of posthog to use pixels instead? We have an immutable 1:1 mapping between pixels and rem-s, so why not update the tailwind config to just output pixels instead?

@daibhin
Copy link
Contributor

daibhin commented Feb 6, 2024

I don't have much context on why we use rem over px beyond knowing that it helps fonts scale. From reading online that seemed to be a bigger issue in years gone by (👀 Internet Explorer) but certainly still affects accessibility if users adjust the side of their fonts for legibility.

Maybe @Twixes knows more since he did the work on Tailwind?

@Twixes
Copy link
Collaborator

Twixes commented Feb 6, 2024

rem is better for accessibility, as people can easily set a larger default font size then in the browser, and the UI becomes appropriately bigger.
Does looks like we can't rely on this for the Toolbar though. We probably also use SCSS in there, so that can be updated right now. As for utility classes, we can pretty easily have a Tailwind config for the toolbar which is an extension of the base one, that only differ by using px instead of rem.

@daibhin
Copy link
Contributor

daibhin commented Feb 6, 2024

Sounds good 👍 I think the only thing that wouldn't account for is custom CSS used in the shared Lemon components. LemonButton comes to mind as an obvious one with a bunch of different rem styles that we wouldn't want to update every time we changed / add a value

@pauldambra
Copy link
Member Author

the 😈 approach would be

when toolbar starts it checks if the font-size < 8px and logs a warning that it might be invisible

then we hope that very few people set font-size smaller on their sites


only 50% joking

@pauldambra
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right feature/toolbar Feature Tag: Toolbar
Development

No branches or pull requests

4 participants