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

Feature/global style option #196

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Feature/global style option #196

merged 4 commits into from
Nov 22, 2021

Conversation

nicmosc
Copy link
Member

@nicmosc nicmosc commented Nov 19, 2021

Allow injectGlobal option of ThemeProvider/DrylusProvider to be disabled when using Drylus in a pre-existing, non-drylus app e.g. Marketplace.

This change makes it so that the styling of drylus-driven components/features is more isolated by making fonts relative to their closest font-setting parent (e.g. ThemeProvider when using the injectGlobal=false option).

If the option is not disabled, then styles are injected globally like before (in fully drylus apps), so nothing is affected visually.

@nicmosc nicmosc self-assigned this Nov 19, 2021
@nicmosc nicmosc added enhancement New feature or request minor Increment the minor version when merged labels Nov 19, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@Bartmr
Copy link
Collaborator

Bartmr commented Nov 19, 2021

Just to confirm, this is a way to avoid styles from a parent app to cascade down to a frame of the screen using drylus right?

@nicmosc
Copy link
Member Author

nicmosc commented Nov 19, 2021

@Bartmr actually it's the opposite: The theme provider from drylus needs to add styling to the html and body elements of a website to make some properties global as that's the standard behaviour of most website. E.g. font-size is defined in the body of the website, and all children use relative values to define their font.
If you use rem (old one) you would base yourself solely on the property set on body, meaning that if the website set a different font size to what drylus is used to (e.g. Marketplace has a font-size of 15px), then things will look strange. Thus we decided to use em which uses the closest parent font-size, and defaults to the body one if that's the only component that sets it.
At the same time, since Theme provider had to set the font-size=14px on the website body, it would override the Marketplace one, which was 15px. Because of this, any of the non-drylus text on marketplace would look really tiny. By using the injectGlobal=false we don't set styles on the body from the Theme provider, meaning any code outside of this component will keep their original styles as the body was not affected

@Bartmr
Copy link
Collaborator

Bartmr commented Nov 19, 2021

@nicmosc Got it. already clicked approve

@nicmosc
Copy link
Member Author

nicmosc commented Nov 22, 2021

@LeopoldArkham ☝️

@nicmosc nicmosc merged commit c9d5ae5 into master Nov 22, 2021
@nicmosc nicmosc deleted the feature/global-style-option branch November 22, 2021 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants