feat(docs): adding theme switcher #280

mnajdova · 2018-09-26T11:07:17Z

Docs - theme switcher

This PR is adding the switching theme feature on the docs site. For this to work, the following things are implemented:

Mobx is added as a dependency for storing the currently selected theme (we are storing just the name of the theme here)
Dropdown menu is added on the sidebar, where all exported themes are displayed as options.
Two additional themes are added: teams dark and teams high contrast theme. These themes are simple and are just adding some styling for the button component, so that we can showcase the theme switching.
The selected theme is merged with the teams default theme in the root Provider of the docs.

Demo:

-added switcher for example // TODO it is merging the both themes

codecov · 2018-09-26T11:15:32Z

Codecov Report

Merging #280 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #280   +/-   ##
=======================================
  Coverage   89.63%   89.63%           
=======================================
  Files          62       62           
  Lines        1167     1167           
  Branches      173      150   -23     
=======================================
  Hits         1046     1046           
  Misses        119      119           
  Partials        2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9e8eae...7e4835b. Read the comment docs.

# Conflicts: # docs/src/routes.tsx

src/themes/teams-dark/components/Button/buttonStyles.ts

…iles of the themes

mnajdova · 2018-09-27T15:36:34Z

Merging is now happening in the export of the theme's index file instead of the Provider. This is ready for final review. @levithomason @kuzhelov @miroslavstastny

layershifter · 2018-09-30T14:03:10Z

Are there any real reason to use mobx there? Possible future plans? It looks like shooting a cannon at sparrows 😸

Possible the better solution will be to use Context API?

mnajdova · 2018-10-01T13:19:40Z

Are there any real reason to use mobx there? Possible future plans? It looks like shooting a cannon at sparrows 😸

Possible the better solution will be to use Context API?

Yeah, we already discussed that the Context API might be a better option in this case. But now that we have mobx integrated already in this PR, and it might be used in other cases, I don't know if we want to remove all that. But yeah, we are adding that dependency without really having actual need for it. I guess we can switch to the Context API if that's what majority of the people think... @kuzhelov @levithomason thoughts?

# Conflicts: # docs/src/routes.tsx

mnajdova · 2018-10-01T17:51:38Z

Let's hold off merging this until we resolve one more issue. Clicking on a component after having the docs running on the introduction page, changes the font size on all page. See the gif below:

It's strange that this is not the same if I directly open some component page in the browser. Will investigate this tomorrow.

mnajdova · 2018-10-01T17:59:01Z

Aah found it, something similar with #221 Will confirm latest changes with @kuzhelov and will merge this.

mnajdova · 2018-10-01T18:10:28Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx

+          ...theme.componentVariables,
+          [component]: {
+            ...(theme.componentVariables && theme.componentVariables[component]),
+            [variable]: value,
          },
        },


@kuzhelov please take a look into the changes in this file regarding the componentVariables. It seems to work okay in my opinion, but would like to have your approval on this, as the initial changes were yours. Thanks!

these changes were made at the moment where there were no mergeThemeVariables yet - now we should rather reuse this functionality here

I changed the state's componentVariable to store just the variables for the component being rendered, and then I am using the mergeThemeVariables for generating the final componentVariables. Please take a look now.

kuzhelov · 2018-10-02T11:35:09Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx


    const newTheme: IThemeInput = {
-      componentVariables: theme.componentVariables,
+      componentVariables,


this means that theme is only about styles that are provided by CSS-like objects. At the same time, it is possible for theme to provide static CSS styles, and those won't be applied with current implementation. Although I do understand that naively applying them will be a source of the problem as well, still, could you, please, suggest what is our vision for this question?

kuzhelov · 2018-10-02T12:09:44Z

package.json

@@ -88,6 +88,8 @@
    "fela-plugin-rtl": "^1.0.6",
    "keyboard-key": "^1.0.1",
    "lodash": "^4.17.10",
+    "mobx": "^5.1.2",


another point to rather use React context for achieving the same goal is that currently we have to include mobx to the list of dependencies - while, essentially, this is used only for docs experience (not something that lib consumer is interested in)

kuzhelov

Just couple of moments that raise my concerns:

what are the plans to handle theme's static CSS
do we really need Mobx? Absolutely agree with @layershifter's sentiments, as couple of additional points
- bundle size of Stardust consumed as a library will increase without any good reason for that (as mobx is used in docs only) - it would be much better if we'll consider to introduce default theme instead for much smaller size cost :)
- even the amount of code necessary will be much lesser if we will use React Context
- there is no immediate need to use Mobx to solve other problems of the library - and in case if there will be any, it is quite straightforward to integrate Mobx when the problem will shape itself, rather than just trying to predict the problems by advertising tool.

…ables for the component styling at that moment -on opening the variables form the merged variables from the theme as well as the state's variables are shown: fixing current issue where the rendered element does not corresponds with the variables values shown in the form

mnajdova · 2018-10-02T15:04:39Z

Mobx is now replaced with react context API. Also, fixed bug for not synchronized form values with variables applied in the example. I hope this is ready now for final review.

kuzhelov · 2018-10-03T09:42:20Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx

@@ -23,17 +23,21 @@ import ComponentExampleTitle from './ComponentExampleTitle'
 import ContributionPrompt from '../ContributionPrompt'
 import getSourceCodeManager, { ISourceCodeManager, SourceCodeType } from './SourceCodeManager'
 import { IThemeInput, IThemePrepared } from 'types/theme'
+import { mergeThemeVariables } from '../../../../../src/lib/mergeThemes'


kuzhelov · 2018-10-03T09:49:58Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx

@@ -123,6 +129,10 @@ class ComponentExample extends React.PureComponent<IComponentExampleProps, IComp
    ) {
      this.clearActiveState()
    }
+    const { themeName } = nextProps
+    if (this.state.themeName !== themeName) {


a bit unsure - do we really need to store themeName as state?

Sorry, this was changed when the name of the theme was fetch from the mobx store, the themeName prop should be enough now. Will change it.

Ah, I got it now. This was added because we want to invoke renderSourceCode if the themeName prop is changed. Since we are invoking this method here, we need to have a way to get the new themeName, that's why I have it in the state as well. Any other idea about how can we achive this?

actually, yes - but just to unblock the PR, we could make a notch, merge it and I will try to experiment on that further. To me themeName has clear immutable semantics (that is not example-specific - and this is a key difference from other ones, like showRtl, for example), so this is the reason I'd rather see it being introduced as prop

kuzhelov · 2018-10-03T09:51:23Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx

@@ -346,10 +356,14 @@ class ComponentExample extends React.PureComponent<IComponentExampleProps, IComp
  private getDisplayName = () => this.props.examplePath.split('/')[1]

  private renderWithProvider(ExampleComponent) {
-    const { showRtl, theme } = this.state
+    const { showRtl, componentVariables } = this.state
+    const { themeName } = this.state


nit: these two lines could be merged into one. However, concerned whether themeName should be part of state

I commented on previous comment.

kuzhelov · 2018-10-03T10:20:48Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx

@@ -540,7 +554,10 @@ class ComponentExample extends React.PureComponent<IComponentExampleProps, IComp
        </Divider>
        <Provider.Consumer
          render={({ siteVariables, componentVariables }: IThemePrepared) => {
-            const variables = componentVariables[displayName]
+            const mergedVariables = mergeThemeVariables(componentVariables, {


should we merge them again here? May be missing something, but it seems that we already merged these variables on line 364, so they will be consumed as already merged ones

It took me also lot of time to figure this out, but this consumer is for the root Theme Provider, where in contrast, the example, uses it's own provider where these are merged (overriden). This is why we actually have previously bug where the componentVariables were not reflecting the latest status of the variables applied on the example.

got it - still, see it to be a problem that we are merging in two different places of the ComponentExample code. Let me take a look on that after PR will be merged

kuzhelov · 2018-10-03T10:30:55Z

docs/src/index.tsx

-      >
-        <NewApp />
-      </Provider>
+      <NewApp />


just to clarify for myself - could you, please, suggest what was the reason of moving (and augmenting) this theme context functionality from here?

This was also moved because of the Mobx initial implementation, but I still think the logic for the new Theme context and the Provider should reside in some component file, rather then here. What are your thoughts on this? Maybe the better approach would be creating an App component for all these rather then having them in the routes, but anyway I would like them in some component logic...

Plus we are using state for the theme context, so we must have some component as a wrapper.

the reason I see the initial implementation as being more semantically correct is that because routes file will correspond to the app-related aspects, and theming context is something that is not closely related to it. This is a subtle question and I cannot state anything here, but would rather omit these changes from the PR for now. Please, feel free to decide by yourself, though :)

Introduced App component that will hold the logic for providing the theme context and the provider.

-reverted commenting of the merged variables in the variables panel

manajdov added 2 commits September 24, 2018 17:55

-added new theme

d368f8d

-added switcher for example // TODO it is merging the both themes

-added docs page theme switcher

f15bbf9

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners September 26, 2018 11:07

mnajdova added the 🚧 WIP label Sep 26, 2018

manajdov added 6 commits September 26, 2018 14:50

-added examples for teams dark and teams high contrast themes

c094e36

-fixed not needed re-rendering when the theme is not changed

3ed0545

-simplifying the high contrast theme

27cf9d1

-simplifying dark and high contrast theme

fa1bec0

-improved styles for the theme picker dropdown

be911d7

-tslint fixes

4dcbc30

mnajdova changed the title ~~[WIP] feat(docs): adding theme switcher~~ feat(docs): adding theme switcher Sep 26, 2018

mnajdova added 🚀 ready for review and removed 🚧 WIP labels Sep 26, 2018

Merge branch 'master' into feat/theme-switcher

0a67747

# Conflicts: # docs/src/routes.tsx

levithomason approved these changes Sep 26, 2018

View reviewed changes

codepretty reviewed Sep 26, 2018

View reviewed changes

src/themes/teams-dark/components/Button/buttonStyles.ts Outdated Show resolved Hide resolved

manajdov added 4 commits September 27, 2018 15:47

Merge branch 'master' into feat/theme-switcher

6455358

-moves the merging of the themes from the Provider ot the exporting f…

e18b8fc

…iles of the themes

-exported just the merged themes

cf9c277

-exported standalone themes (dark and HC)

cfd9dac

levithomason added 3 commits September 27, 2018 13:08

Merge branch 'master' into feat/theme-switcher

d6ccfb3

cleanup sidebar

a62497e

cleanup dark/contrast theme values

a032a55

manajdov added 3 commits October 1, 2018 18:53

Merge branch 'master' into feat/theme-switcher

6d35bb3

-removing static styles for the semantic ui dropdown

2933d8f

Merge branch 'master' into feat/theme-switcher

de34860

# Conflicts: # docs/src/routes.tsx

manajdov added 2 commits October 1, 2018 19:59

-fixing issue with changing body font size

090364b

-removing unused variables

e7d7ca3

mnajdova commented Oct 1, 2018

View reviewed changes

kuzhelov reviewed Oct 2, 2018

View reviewed changes

manajdov added 3 commits October 2, 2018 15:15

-replaced mobx with react context API

60c8720

-reverted tsconfig to the initial version

a08e6b7

manajdov added 3 commits October 2, 2018 17:47

Merge branch 'master' into feat/theme-switcher

94f5942

Merge branch 'master' into feat/theme-switcher

9a71f3d

-updated changelog

6f91a46

kuzhelov reviewed Oct 3, 2018

View reviewed changes

-merging two lines

426fd71

kuzhelov approved these changes Oct 3, 2018

View reviewed changes

manajdov added 2 commits October 3, 2018 14:29

-introduced app component as a root for the docs

68eb2b1

-reverted commenting of the merged variables in the variables panel

Merge branch 'master' into feat/theme-switcher

7e4835b

mnajdova merged commit 5927d6c into master Oct 3, 2018

levithomason deleted the feat/theme-switcher branch October 11, 2018 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(docs): adding theme switcher #280

feat(docs): adding theme switcher #280

mnajdova commented Sep 26, 2018 •

edited

codecov bot commented Sep 26, 2018 •

edited

mnajdova commented Sep 27, 2018

layershifter commented Sep 30, 2018 •

edited

mnajdova commented Oct 1, 2018

mnajdova commented Oct 1, 2018

mnajdova commented Oct 1, 2018

mnajdova Oct 1, 2018

kuzhelov Oct 2, 2018

mnajdova Oct 2, 2018

kuzhelov Oct 2, 2018 •

edited

kuzhelov Oct 2, 2018

kuzhelov left a comment

mnajdova commented Oct 2, 2018

kuzhelov Oct 3, 2018

kuzhelov Oct 3, 2018

mnajdova Oct 3, 2018

mnajdova Oct 3, 2018

kuzhelov Oct 3, 2018

kuzhelov Oct 3, 2018 •

edited

mnajdova Oct 3, 2018

kuzhelov Oct 3, 2018

mnajdova Oct 3, 2018

kuzhelov Oct 3, 2018

kuzhelov Oct 3, 2018

mnajdova Oct 3, 2018

mnajdova Oct 3, 2018

kuzhelov Oct 3, 2018

mnajdova Oct 3, 2018

feat(docs): adding theme switcher #280

feat(docs): adding theme switcher #280

Conversation

mnajdova commented Sep 26, 2018 • edited

Docs - theme switcher

Demo:

codecov bot commented Sep 26, 2018 • edited

Codecov Report

mnajdova commented Sep 27, 2018

layershifter commented Sep 30, 2018 • edited

mnajdova commented Oct 1, 2018

mnajdova commented Oct 1, 2018

mnajdova commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov left a comment

Choose a reason for hiding this comment

mnajdova commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Sep 26, 2018 •

edited

codecov bot commented Sep 26, 2018 •

edited

layershifter commented Sep 30, 2018 •

edited

kuzhelov Oct 2, 2018 •

edited

kuzhelov Oct 3, 2018 •

edited