-
Notifications
You must be signed in to change notification settings - Fork 359
fix(clerk-js): CSS Variables #6100
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
base: main
Are you sure you want to change the base?
Changes from all commits
4276ecd
167b12c
a33cec6
eb9f682
f7ebfcc
3569d15
23316c5
a1299d6
3bf7284
94cb7b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@clerk/clerk-js': minor | ||
--- | ||
|
||
[TODO] | ||
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,8 +13,7 @@ import { Tooltip } from '@/ui/elements/Tooltip'; | |||||||||||||||||||||||||||||
import { LockDottedCircle } from '@/ui/icons'; | ||||||||||||||||||||||||||||||
import { Textarea } from '@/ui/primitives'; | ||||||||||||||||||||||||||||||
import type { ThemableCssProp } from '@/ui/styledSystem'; | ||||||||||||||||||||||||||||||
import { common } from '@/ui/styledSystem'; | ||||||||||||||||||||||||||||||
import * as utils from '@/ui/utils'; | ||||||||||||||||||||||||||||||
import { colorMix } from '@/ui/utils/colorMix'; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
export function OAuthConsentInternal() { | ||||||||||||||||||||||||||||||
const { scopes, oAuthApplicationName, oAuthApplicationLogoUrl, redirectUrl, onDeny, onAllow } = | ||||||||||||||||||||||||||||||
|
@@ -121,10 +120,7 @@ export function OAuthConsentInternal() { | |||||||||||||||||||||||||||||
<Box | ||||||||||||||||||||||||||||||
sx={t => ({ | ||||||||||||||||||||||||||||||
padding: t.space.$3, | ||||||||||||||||||||||||||||||
background: common.mergedColorsBackground( | ||||||||||||||||||||||||||||||
utils.colors.setAlpha(t.colors.$colorBackground, 1), | ||||||||||||||||||||||||||||||
t.colors.$neutralAlpha50, | ||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||
backgroundColor: colorMix(t.colors.$colorBackground, t.colors.$neutralAlpha50), | ||||||||||||||||||||||||||||||
})} | ||||||||||||||||||||||||||||||
Comment on lines
121
to
124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainColor-mix may break on non-Chromium browsers – add fallback
backgroundColor: colorMix(t.colors.$colorBackground, t.colors.$neutralAlpha50),
+backgroundColor: [
+ colorMix(t.colors.$colorBackground, t.colors.$neutralAlpha50),
+ t.colors.$neutralAlpha50, // fallback for browsers without color-mix
+].join(','), This keeps the new look while preserving graceful degradation. 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Search for occurrences of `colorMix` in the codebase
grep -R "colorMix" -n . Length of output: 65620 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Display first 200 lines of the colorMix utility to verify its output
sed -n '1,200p' packages/clerk-js/src/ui/utils/colorMix.ts Length of output: 5964 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Show lines 1-200 of the OAuthConsent.tsx for context
sed -n '1,200p' packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx Length of output: 7269 Add a solid fallback before using colorMix for unsupported browsers To avoid an invalid sx={t => ({
padding: t.space.$3,
- backgroundColor: colorMix(t.colors.$colorBackground, t.colors.$neutralAlpha50),
+ // fallback for browsers without color-mix
+ backgroundColor: t.colors.$neutralAlpha50,
+ backgroundColor: colorMix(t.colors.$colorBackground, t.colors.$neutralAlpha50),
})} Please apply the same pattern wherever • packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx Consider extracting a helper or theme token if this becomes a widespread pattern. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This package only supports browsers released in the last two years (since May 8 2023), so all of our supported browsers support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dstaley Thank you for the clarification! I appreciate you letting me know about the package's browser support policy. That makes perfect sense - if you're only supporting browsers from the last two years, then ✏️ Learnings added
|
||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||
<Text | ||||||||||||||||||||||||||||||
|
@@ -333,10 +329,7 @@ function ConnectionIcon({ size = 'md', sx }: { size?: 'sm' | 'md'; sx?: Themable | |||||||||||||||||||||||||||||
<Box | ||||||||||||||||||||||||||||||
sx={t => [ | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
background: common.mergedColorsBackground( | ||||||||||||||||||||||||||||||
utils.colors.setAlpha(t.colors.$colorBackground, 1), | ||||||||||||||||||||||||||||||
t.colors.$neutralAlpha50, | ||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||
backgroundColor: colorMix(t.colors.$colorBackground, t.colors.$neutralAlpha50), | ||||||||||||||||||||||||||||||
borderRadius: t.radii.$circle, | ||||||||||||||||||||||||||||||
borderWidth: t.borderWidths.$normal, | ||||||||||||||||||||||||||||||
borderStyle: t.borderStyles.$solid, | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,25 +2,33 @@ import type { Theme } from '@clerk/types'; | |||||||||
|
||||||||||
import { spaceScaleKeys } from '../foundations/sizes'; | ||||||||||
import type { fontSizes, fontWeights } from '../foundations/typography'; | ||||||||||
import { fromEntries, removeUndefinedProps } from '../utils'; | ||||||||||
import { | ||||||||||
colorOptionToHslaAlphaScale, | ||||||||||
colorOptionToHslaLightnessScale, | ||||||||||
colors, | ||||||||||
fromEntries, | ||||||||||
removeUndefinedProps, | ||||||||||
} from '../utils'; | ||||||||||
createAlphaScaleWithTransparentize, | ||||||||||
createColorMixLightnessScale, | ||||||||||
lighten, | ||||||||||
transparentize, | ||||||||||
} from '../utils/colorMix'; | ||||||||||
|
||||||||||
export const createColorScales = (theme: Theme) => { | ||||||||||
const variables = theme.variables || {}; | ||||||||||
|
||||||||||
const primaryScale = colorOptionToHslaLightnessScale(variables.colorPrimary, 'primary'); | ||||||||||
const primaryAlphaScale = colorOptionToHslaAlphaScale(primaryScale?.primary500, 'primaryAlpha'); | ||||||||||
const dangerScale = colorOptionToHslaLightnessScale(variables.colorDanger, 'danger'); | ||||||||||
const dangerAlphaScale = colorOptionToHslaAlphaScale(dangerScale?.danger500, 'dangerAlpha'); | ||||||||||
const successScale = colorOptionToHslaLightnessScale(variables.colorSuccess, 'success'); | ||||||||||
const successAlphaScale = colorOptionToHslaAlphaScale(successScale?.success500, 'successAlpha'); | ||||||||||
const warningScale = colorOptionToHslaLightnessScale(variables.colorWarning, 'warning'); | ||||||||||
const warningAlphaScale = colorOptionToHslaAlphaScale(warningScale?.warning500, 'warningAlpha'); | ||||||||||
const primaryScale = createColorMixLightnessScale(variables.colorPrimary, 'primary'); | ||||||||||
const primaryBase = primaryScale?.primary500; | ||||||||||
const primaryAlphaScale = primaryBase ? createAlphaScaleWithTransparentize(primaryBase, 'primaryAlpha') : undefined; | ||||||||||
const dangerScale = createColorMixLightnessScale(variables.colorDanger, 'danger'); | ||||||||||
const dangerBase = dangerScale?.danger500; | ||||||||||
const dangerAlphaScale = dangerBase ? createAlphaScaleWithTransparentize(dangerBase, 'dangerAlpha') : undefined; | ||||||||||
const successScale = createColorMixLightnessScale(variables.colorSuccess, 'success'); | ||||||||||
const successBase = successScale?.success500; | ||||||||||
const successAlphaScale = successBase ? createAlphaScaleWithTransparentize(successBase, 'successAlpha') : undefined; | ||||||||||
const warningScale = createColorMixLightnessScale(variables.colorWarning, 'warning'); | ||||||||||
const warningBase = warningScale?.warning500; | ||||||||||
const warningAlphaScale = warningBase ? createAlphaScaleWithTransparentize(warningBase, 'warningAlpha') : undefined; | ||||||||||
const neutralAlphaScales = | ||||||||||
typeof variables.colorNeutral === 'string' && variables.colorNeutral | ||||||||||
? createAlphaScaleWithTransparentize(variables.colorNeutral, 'neutralAlpha') | ||||||||||
: {}; | ||||||||||
|
||||||||||
return removeUndefinedProps({ | ||||||||||
...primaryScale, | ||||||||||
|
@@ -31,22 +39,20 @@ export const createColorScales = (theme: Theme) => { | |||||||||
...successAlphaScale, | ||||||||||
...warningScale, | ||||||||||
...warningAlphaScale, | ||||||||||
...colorOptionToHslaAlphaScale(variables.colorNeutral, 'neutralAlpha'), | ||||||||||
primaryHover: colors.adjustForLightness(primaryScale?.primary500), | ||||||||||
colorTextOnPrimaryBackground: toHSLA(variables.colorTextOnPrimaryBackground), | ||||||||||
colorText: toHSLA(variables.colorText), | ||||||||||
colorTextSecondary: toHSLA(variables.colorTextSecondary) || colors.makeTransparent(variables.colorText, 0.35), | ||||||||||
colorInputText: toHSLA(variables.colorInputText), | ||||||||||
colorBackground: toHSLA(variables.colorBackground), | ||||||||||
colorInputBackground: toHSLA(variables.colorInputBackground), | ||||||||||
colorShimmer: toHSLA(variables.colorShimmer), | ||||||||||
...neutralAlphaScales, | ||||||||||
// TODO(Colors): We are not adjusting the lightness based on the colorPrimary lightness | ||||||||||
primaryHover: primaryBase ? lighten(primaryBase, '90%') : undefined, | ||||||||||
colorTextOnPrimaryBackground: variables.colorTextOnPrimaryBackground, | ||||||||||
Comment on lines
+44
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion
-primaryHover: primaryBase ? lighten(primaryBase, '90%') : undefined,
+primaryHover: primaryBase ? lighten(primaryBase, '10%') : undefined, or clarify with a named helper ( 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||
colorText: variables.colorText, | ||||||||||
colorTextSecondary: | ||||||||||
variables.colorTextSecondary || (variables.colorText ? transparentize(variables.colorText, '35%') : undefined), | ||||||||||
colorInputText: variables.colorInputText, | ||||||||||
colorBackground: variables.colorBackground, | ||||||||||
colorInputBackground: variables.colorInputBackground, | ||||||||||
colorShimmer: variables.colorShimmer, | ||||||||||
}); | ||||||||||
}; | ||||||||||
|
||||||||||
export const toHSLA = (str: string | undefined) => { | ||||||||||
return str ? colors.toHslaString(str) : undefined; | ||||||||||
}; | ||||||||||
|
||||||||||
export const createRadiiUnits = (theme: Theme) => { | ||||||||||
const { borderRadius } = theme.variables || {}; | ||||||||||
if (borderRadius === undefined) { | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Populate the changeset before merging
[TODO]
will reach the changelog verbatim, leaving consumers without any context for the minor release. Replace it with a concise summary of the CSS-variable migration and colour-utility refactor.🤖 Prompt for AI Agents