Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(docs): font size issue#221

Merged
levithomason merged 8 commits intomasterfrom
fix/docs-radio-font-size
Sep 14, 2018
Merged

fix(docs): font size issue#221
levithomason merged 8 commits intomasterfrom
fix/docs-radio-font-size

Conversation

@kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Sep 13, 2018

Repro steps:

Expected result

  • default font size remains to be 14px
    image

Actual result

  • default font size is switched on 10px
    image

Cause:

The issue is caused by the following tree being rendered:

<Provider theme={mergeThemes(teams,
  siteVariables: {
    ...
   htmlFontSize: '14px'
 })}
 ...
  <ComponentExample>
     <Provider theme={mergeThemes(teams, newTheme)}>
     ...
     </Provider>
   </ComponentExample>
} />

As we might see, when component example is rendered, teams theme as applied once again - with all its static styles that invalidate htmlFontSize: 14px override and set htmlFontSize to 10px, the value defined by teams theme.

Provided fix eliminates specifying theme for individual example, as we would rather like to apply theme changes for all examples altogether.

Broader issue to fix

It is a very dangerous phenomena related to the fact that theme applied to subtree of components might change look of the overall page - with global CSS selectors being used in its static styles. More than that, this problem is very hard to diagnose, and for some situations I might imagine that it will be even hard to spot. I would propose to apply mechanisms of scoping static CSS selectors to the subtree theme as applied for (by default), with opened possibility to explicitly allow global CSS for Provider:

{ /*global CSS selectors are allowed, so that theme could change overall page's look */ }
<Provider theme={teams} allowGlobalCss>  
  ...
  { /* static CSS styles provided will be scoped to Provider's subtree only */ }
  <Provider theme={someTheme}> 
     ...
  </Provider>
...
</Provider>

CHANGELOG.md Outdated

### Fixes
- Fix `Popup` component imports @mnajdova ([#222](https://github.com/stardust-ui/react/pull/222))
- Fix font size issue in docs @kuzhelov ([#221](https://github.com/stardust-ui/react/pull/221))
Copy link
Member

@levithomason levithomason Sep 14, 2018

Choose a reason for hiding this comment

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

I don't think we should advertise doc changes here, only library changes. Thoughts on keeping the changelog specific to the library itself?

CHANGELOG.md Outdated
## [Unreleased]

### Fixes
- Fix `Popup` component imports @mnajdova ([#222](https://github.com/stardust-ui/react/pull/222))
Copy link
Member

Choose a reason for hiding this comment

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

Cruft entry here.

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

Some cleanup is due here.

@levithomason levithomason added needs author changes Author needs to implement changes before merge and removed ready for merge labels Sep 14, 2018
@levithomason
Copy link
Member

Agree on the broader issue. @davezuko and I also discussed this similar issue. There is also the converse case that something rendered down the tree might need to add statics or font faces in order to work.

@levithomason levithomason added 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Sep 14, 2018
@levithomason levithomason merged commit f5d78be into master Sep 14, 2018
@levithomason levithomason deleted the fix/docs-radio-font-size branch September 14, 2018 05:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🧰 fix Introduces fix for broken behavior. 🚀 ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants