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.

Sign up for GitHub

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

Jump to bottom

[Joy] theme setup #29846

Merged
merged 94 commits into from
Nov 30, 2021
Merged

[Joy] theme setup #29846

merged 94 commits into from
Nov 30, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Nov 23, 2021 •
edited
Loading

Demo: https://codesandbox.io/s/joy-cra-typescript-forked-2jzli?file=/src/App.js

The demo showcase how the default theme can be used to build the design system.

Summary

  • set up default theme according to Notion.
    • breakpoints & spacing is provided from the system package
    • naming follow most of mui-material. This will benefit mui-material developers who want to try Joy. This also makes most of the shorthands work within sx prop.
    • some typing improvements.

Note

  • Names can be discussed here but it should not block the PR (if there seems to be a long discussion)
  • Please review defaultTheme.ts and the test cases.

@siriwatknp
add colorSchemes to theme
ea81bc8
@siriwatknp
add color schemes test
a43b956
@siriwatknp
init marketing page
1da7eb4
@siriwatknp
fix types
e9e2c5f
@siriwatknp
fix default system mode
8b7b3af
@siriwatknp
log on screen
d478c2d
@siriwatknp
add single quote
f6fed06
@siriwatknp
remove log
95a5779
@siriwatknp
adjust styles
7aa42ad
@siriwatknp
add joy-dashboard page
a7d1027
@siriwatknp
apply all color schemees
8b87e85
@siriwatknp
fix purple color
5e79f37
@siriwatknp
add contained context vars
89f53dc
@siriwatknp
Merge branch 'master' of https://github.com/mui-org/material-ui into …
fcb056c 
…joy/marketing-mockup
@siriwatknp
move to experiments
3a7ff40
@siriwatknp
add variants experiment
b86e3c7
@siriwatknp
add variants page
83b475f
@siriwatknp
adjust context variables based on color scheme
c95cbb0
@siriwatknp
add sketching page
58f5602
@siriwatknp
remove unused components
a2fc13f
@siriwatknp
add themes page
613518b
@siriwatknp
add e-commerce page
a8b7049
@siriwatknp
add focus and elevation
fe4b58a
@siriwatknp
fix elevation css bug
e006597
@siriwatknp
Merge branch 'master' of https://github.com/mui-org/material-ui into …
4a98350 
…joy/marketing-mockup
@siriwatknp
add borderRadius to default theme
c19702d
@siriwatknp
add elevation to default theme
ee3ccfa
@siriwatknp
add focus to default theme
80614ef
@siriwatknp
add variant to default theme
66f3948
@siriwatknp
remove shadow
f8ee4ea
@siriwatknp siriwatknp force-pushed the joy/theme-implementation branch 2 times, most recently from 0efd14a to a3806f2 Compare November 25, 2021 05:57
@siriwatknp siriwatknp force-pushed the joy/theme-implementation branch from a3806f2 to c0597f0 Compare November 25, 2021 06:05
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 25, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 26, 2021
@siriwatknp siriwatknp added the on hold There is a blocker, we need to wait label Nov 26, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 29, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 29, 2021
@siriwatknp siriwatknp removed the on hold There is a blocker, we need to wait label Nov 29, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 29, 2021
danilo-leal
danilo-leal reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

A few questions and some changes. Great work with the setup, Jun!

packages/mui-joy/src/styles/ColorSystem.ts Outdated
| 'info'
| 'success'
| 'warning'
| 'context';
Copy link
Contributor

Choose a reason for hiding this comment

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

What's context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Components can change the color to context to enable the contextual override feature. This is added after chatting with @oliviertassinari because it makes the variables easier to reason. More detail https://www.notion.so/mui-org/RFC-Theme-implementation-e15bc2ef11ba4f6f804434af27141afe#cdcb3ea92fcc44d798d7de6a77971537

packages/mui-joy/src/styles/defaultTheme.ts
Comment on lines +216 to +217
secondary: 'var(--joy-palette-neutral-600)',
tertiary: 'var(--joy-palette-neutral-500)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
secondary: 'var(--joy-palette-neutral-600)',
tertiary: 'var(--joy-palette-neutral-500)',
secondary: 'var(--joy-palette-neutral-700)',
tertiary: 'var(--joy-palette-neutral-600)',

A little bump on these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure? I still think the tertiary should be 500 because the contrast still works with neutral.100

image

changing secondary to 700 is too close to the primary in my opinion.

I think we should go with the existing one and try it out first, if you think this does not work we can change it before the first release.

packages/mui-joy/src/styles/defaultTheme.ts Outdated Show resolved Hide resolved
packages/mui-joy/src/styles/defaultTheme.ts Outdated Show resolved Hide resolved
packages/mui-joy/src/styles/defaultTheme.ts Outdated Show resolved Hide resolved
packages/mui-joy/src/styles/defaultTheme.ts Outdated
sans: '"Public Sans", Roboto',
mono: 'Consolas',
default: '"Public Sans", var(--joy-fontFamily-fallback)',
display: '"PlusJakartaSans-ExtraBold", var(--joy-fontFamily-fallback)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep Public Sans as the typeface for the display typography on the default theme? JakartaSans is only meant for Joy marketing materials, so we could override the default theme to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, no problem.

packages/mui-joy/src/styles/defaultTheme.ts Outdated Show resolved Hide resolved
packages/mui-joy/src/styles/defaultTheme.ts Show resolved Hide resolved
packages/mui-joy/src/Button/Button.tsx Outdated Show resolved Hide resolved
packages/mui-joy/src/Button/Button.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 30, 2021
@siriwatknp siriwatknp merged commit c82145c into mui:master Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danilo-leal danilo-leal danilo-leal left review comments

@mnajdova mnajdova mnajdova approved these changes

@hbjORbj hbjORbj Awaiting requested review from hbjORbj

@oliviertassinari oliviertassinari Awaiting requested review from oliviertassinari

Assignees
No one assigned
Labels
package: joy-ui Specific to @mui/joy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@siriwatknp @mui-pr-bot @mnajdova @danilo-leal