refactor: improve stage-web color palette settings readability#1671
refactor: improve stage-web color palette settings readability#1671drHuangMHT wants to merge 10 commits intomoeru-ai:mainfrom
Conversation
…web-color-palette
There was a problem hiding this comment.
Code Review
This pull request centralizes transition colors using CSS variables and introduces a useCssVariables composable with accompanying unit tests. Feedback highlights a reactivity bug in App.vue where colors may become stale when the theme hue changes. Significant logic and compatibility issues were identified in the useCssVariables composable regarding target cleanup and ES2024 features, and corrections to the JSDoc examples were suggested to ensure type safety and maintain reactivity.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d02ab3a7ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c025a503b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| getCSSVar('--primary-color'), | ||
| getCSSVar('--secondary-color'), | ||
| getCSSVar('--tertiary-color'), |
There was a problem hiding this comment.
Keep transition colors reactive during dynamic hue animation
This computed now snapshots resolved CSS colors via getComputedStyle(...), so it only updates when tracked refs change; however dynamic hue mode animates --chromatic-hue in CSS (.dynamic-hue) without changing settings.themeColorsHue. As a result, when dynamic hue is enabled, transitions use stale sampled colors instead of tracking the live hue animation that the previous color-expression approach provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
who would ever want an interface with flashy RGB effects🤦 does the original code also watch this change? @codex
There was a problem hiding this comment.
the css variable returns what's in colors.css as raw value that is not mixed into RGB value, which is static. this is not an issue as long as the variable is used in styles. CSS engine will handle the update automatically.
|
What is the purpose of this PR? If you need to adjust the color, perhaps reading https://proj-airi-chromatic.netlify.app/ |
|
If possible, submit more screenshots, explaining why this is necessary, then re-open another PR if needed. |
I don't think screenshots are necessary because the code does not involve changes to the visual appearance of the application. I'm just trying to organize the code for readability. It's about the program code, about the developer experience, not the appearance of the application, not the color-mixing algorithms. |
|
@nekomeowww not re-opening this one? |
| :root { | ||
| /* Light mode defaults */ | ||
| --chromatic-hue: 0; | ||
| --chromatic-chroma-900: 0.15; |
There was a problem hiding this comment.
This is controlled and injected by https://github.com/proj-airi/chromatic. We should not define this here.
| const primaryColor = computed(() => { | ||
| return isDark.value | ||
| ? `color-mix(in srgb, oklch(95% var(--chromatic-chroma-900) calc(var(--chromatic-hue) + ${0})) 70%, oklch(50% 0 360))` | ||
| : `color-mix(in srgb, oklch(95% var(--chromatic-chroma-900) calc(var(--chromatic-hue) + ${0})) 90%, oklch(90% 0 360))` | ||
| }) | ||
|
|
||
| const secondaryColor = computed(() => { | ||
| return isDark.value | ||
| ? `color-mix(in srgb, oklch(95% var(--chromatic-chroma-900) calc(var(--chromatic-hue) + ${180})) 70%, oklch(50% 0 360))` | ||
| : `color-mix(in srgb, oklch(95% var(--chromatic-chroma-900) calc(var(--chromatic-hue) + ${180})) 90%, oklch(90% 0 360))` | ||
| }) | ||
|
|
||
| const tertiaryColor = computed(() => { | ||
| return isDark.value | ||
| ? `color-mix(in srgb, oklch(95% var(--chromatic-chroma-900) calc(var(--chromatic-hue) + ${60})) 70%, oklch(50% 0 360))` | ||
| : `color-mix(in srgb, oklch(95% var(--chromatic-chroma-900) calc(var(--chromatic-hue) + ${60})) 90%, oklch(90% 0 360))` | ||
| }) |
There was a problem hiding this comment.
This should be replaced with the new https://github.com/proj-airi/chromatic/blob/3be1f5990069fa5dd006ae314c979ef54906d1e5/packages/chromatic/src/index.ts#L148-L198
| const { | ||
| primaryColor = '#666', | ||
| secondaryColor = '#ccc', | ||
| zIndex = 100, | ||
| } = defineProps<Omit<StageTransitionCommonParams, 'name'>>() |
There was a problem hiding this comment.
withDefaults should be used as https://vuejs.org/guide/typescript/composition-api.html#props-default-values explained.
There was a problem hiding this comment.
we are using vue ^3.5.32 according to the catalog, and reactive props destructure is available. not using it?
| */ | ||
| export function useCssVariables( | ||
| variableGetter: MaybeRefOrGetter<Record<string, string | undefined>>, | ||
| options?: { elementGetter?: MaybeRefOrGetter<HTMLElement | null>, prefix?: string }, |
There was a problem hiding this comment.
| options?: { elementGetter?: MaybeRefOrGetter<HTMLElement | null>, prefix?: string }, | |
| options?: { target?: MaybeRefOrGetter<HTMLElement | null>, prefix?: string }, |
| useCssVariables(() => ({ | ||
| 'overlay-color-1': primaryColor, | ||
| 'overlay-color-2': secondaryColor, | ||
| }), { prefix: '--stage-transition-3-' }) |
There was a problem hiding this comment.
What is the purpose of this?
There was a problem hiding this comment.
improve code readability. useCssVariables also handles reactivity when you need it(although not needed in this case).
| const { | ||
| colors = ['#eee', '#ebcb8b', '#c56370', '#3f3b52'], | ||
| delay = 0, | ||
| duration = 0.4, | ||
| zIndex = 100, | ||
| } = defineProps<TransitionParams>() |
There was a problem hiding this comment.
withDefaults should be used as https://vuejs.org/guide/typescript/composition-api.html#props-default-values explained.
| const { | ||
| direction = 'up', | ||
| primaryColor = '#eee', | ||
| duration = 0.6, | ||
| delay = 0, | ||
| borderRadius = { sm: '14rem', md: '14rem', lg: '50%' }, | ||
| zIndex = 100, | ||
| } = defineProps<TransitionParams>() |
There was a problem hiding this comment.
withDefaults should be used as https://vuejs.org/guide/typescript/composition-api.html#props-default-values explained.
Description
Improve code readability:
useCssVariablescomposable(with proper tests, and some features!)packages introduced:
@vue/test-utilsandjsdomfor running tests(dev dependency)Linked Issues
Additional Context