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

Add theme hook #120

Merged
merged 13 commits into from
Sep 28, 2021
Merged

Add theme hook #120

merged 13 commits into from
Sep 28, 2021

Conversation

shangzhel
Copy link
Collaborator

Adds:

  • Palette
  • Shapes
  • Font families
  • Font weights
  • Font sizes
  • Icons
  • Code samples (does anyone actually want this?)

@shangzhel shangzhel added the enhancement New feature or request label Sep 27, 2021
@shangzhel shangzhel added this to the Sprint 2 milestone Sep 27, 2021
@shangzhel shangzhel self-assigned this Sep 27, 2021
Copy link
Owner

@chomosuke chomosuke left a comment

Choose a reason for hiding this comment

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

I'm guessing useTheme will return default theme unless overwritten by some ThemeContext higher up in the hierachy. Therefore we should always use useTheme to get our theme.

Unless we explicitly want the default theme.

Copy link
Collaborator

@oldbugo oldbugo left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the implementation. One thing we might want to consider is changing the naming scheme for the font-size from 16, 24, 36, to small, standard, title, just so that if we were to change the size itself the naming convention won't need changing. But I also think this naming may be confusing to use so I'll leave it up to you if want to make this small change. Approved either way.

Copy link
Collaborator

@oldbugo oldbugo left a comment

Choose a reason for hiding this comment

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

I've noticed I have missed some icons in the style guide, so after discussing with Richard we made some changes together to include the new icons and deleted some no-longer-used icons. And we made some changes to naming scheme of the variables to be more flexible if we were to change it in the future. Please have a look at richard/theme-hook

Copy link
Collaborator

@oldbugo oldbugo left a comment

Choose a reason for hiding this comment

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

Thanks

@shangzhel shangzhel merged commit e279244 into master Sep 28, 2021
@shangzhel shangzhel deleted the shangzhel/theme-hook branch September 28, 2021 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add theme hook
4 participants