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

multi: Add Themes on Pi app. #2742

Merged
merged 5 commits into from Mar 30, 2022
Merged

Conversation

victorgcramos
Copy link
Member

Our current politeia app depends on the pi-ui theme provider to format
fonts, colors and styles.

In order to persist theme configuration, all pages should get values from
the same theme. However, each route has its own ReactDOM renderer, hence,
all routes would have its own theme config if we use the ThemeProvider from pi-ui.
That's because ThemeProvider uses Context API for state management.

The core idea behind this diff is to use the same theme config among the whole
app, therefore, use values from the redux-store instead of the context state.

Screen Shot 2022-03-22 at 2 28 59 PM

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

I agree with moving the theme control to redux so we can access the theme name and switch themes from anywhere in the app. But is there a reason to save all the theme variables in the redux store? That seems unnecessary to me. Reasons:

  1. It will never change at runtime. Themes are defined by the developer before building the app. We don't allow the user to choose a new primary color, for example.
  2. Too much info to be saved in the store while we could just save the theme name and access variables in a theme object (like pi-ui does).
  3. What I would do: store only current theme and theme options in the store.

Unless we are planning to allow users to customize their apps looks at runtime, I think saving theme variables in the redux store is a bad idea.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

LGTM

@tiagoalvesdulce tiagoalvesdulce merged commit 17b95e2 into decred:master Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants