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

PR for Milestone 1: Adding the page-color-scheme #17

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

tingting-lwy
Copy link

PR for Milestone 1: Adding the page-color-scheme

  • Added a property data-theme-color that can take either manifold-scheme-light or manifold-scheme-dark
  • Included the Scheme utility classes .manifold-scheme-dark and .manifold-scheme-light
  • Used window.matchMedia to query for user's browser OS preference (if set to dark, will choose manifold-scheme-dark, if light then manifold-scheme-light)
  • Provided input as a dropdown selector to choose the theme on the configurator UI
  • Added the configuration option of page color scheme to Restricted Token and Wallet Identity Widgets too, but not sure if that is necessary as those widgets aren't displayed

An example of the dark theme:
image
image

An example of the light theme
image
image

@jaxonL
Copy link
Contributor

jaxonL commented Apr 10, 2023

linking first pass review for reference tingting-lwy#1 (review)

Copy link
Contributor

@jaxonL jaxonL left a comment

Choose a reason for hiding this comment

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

looks good to me! if anything needs improvement, it would be putting the widgetTheme options as a default for any widget, so we wouldn't need to redefine the prop in each newly added widget config. for now, this is sufficient.

Comment on lines +12 to +16
interface WidgetThemeDefinition {
value: string;
options: WidgetPropOptions[];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this works -- was thinking since we have set values for the theme stuff (the scheme utility classes), we can be more explicit about what a theme is.

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.

2 participants