feat(ui): Initial AppearanceProvider support #3976
Conversation
🦋 Changeset detectedLatest commit: 1ecaaed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| <AppearanceProvider appearance={{ layout: { unsafe_disableDevelopmentModeWarnings: devMode === 'off' } }}> | ||
| <style | ||
| dangerouslySetInnerHTML={{ | ||
| __html: css, |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| /** | ||
| * Union of all valid descriptors used throughout the components. | ||
| */ | ||
| export type DescriptorIdentifier = 'alert' | 'alert__error' | 'alert__warning' | 'alertRoot' | 'alertIcon'; |
There was a problem hiding this comment.
Will definitely want to work out an approach for naming here. I think this was just for testing to start. But ideally we have a convention for underscore vs camelcase etc.
| export const layoutStyle = { | ||
| alert: { | ||
| className: 'border px-4 py-3', | ||
| }, | ||
| alert__warning: {}, | ||
| alert__error: {}, | ||
| alertRoot: { | ||
| className: 'flex gap-x-2', | ||
| }, | ||
| alertIcon: { | ||
| className: 'mt-px shrink-0 *:size-4', | ||
| }, | ||
| } satisfies ParsedElementsFragment; | ||
|
|
||
| export const visualStyle = { | ||
| alert: { | ||
| className: 'leading-small rounded-md text-base', | ||
| }, | ||
| alert__warning: { | ||
| className: 'text-warning bg-warning/[0.06] border-warning/[0.12]', | ||
| }, | ||
| alert__error: { | ||
| className: 'text-danger bg-danger/[0.06] border-danger/[0.12]', | ||
| }, | ||
| alertRoot: {}, | ||
| alertIcon: {}, | ||
| } satisfies ParsedElementsFragment; |
There was a problem hiding this comment.
Since both the layout and visual styles are needed for each component (I think), have you considered a helper function that'd solve some of the need to have empty objects defined here?
There was a problem hiding this comment.
Will also have a good amount of these objects for files like the card component which has a good amount of nested components https://github.com/clerk/javascript/blob/main/packages/ui/src/primitives/card.tsx
There was a problem hiding this comment.
The empty objects are a type checking requirement, but after talking with Bryce we decided to move to a runtime validation approach instead. In a future PR, we'll be able to remove these empty objects, and our useAppearance hook will automatically create the empty objects without us needing to specify them.
| } from '~/contexts/AppearanceContext'; | ||
| import * as Icon from './icon'; | ||
|
|
||
| export const layoutStyle = { |
There was a problem hiding this comment.
I am still not super sold on the layout naming for the based styles. But we can start with it and iterate if we want to change.
| function FirstFactorConnections({ | ||
| isGlobalLoading, | ||
| hasConnection, | ||
| }: { | ||
| isGlobalLoading: boolean; | ||
| hasConnection: boolean; | ||
| }) { | ||
| const { t } = useLocalizations(); |
There was a problem hiding this comment.
Are these changes meant to be here? 👀
There was a problem hiding this comment.
Ah, I see yes! Alex had a few PRs targeting this branch 👍
|
|
||
| export { AppearanceProvider, useAppearance }; | ||
|
|
||
| if (import.meta.vitest) { |
There was a problem hiding this comment.
love that you can test local functions without having to export them!
| return result; | ||
| } | ||
|
|
||
| function mergeAppearance(a: Appearance | null | undefined, b: Appearance | null | undefined): Appearance | null { |
There was a problem hiding this comment.
Might be helpful to outline the merging heuristic in prose in a JSDoc. 👀
Co-authored-by: Alex Carpenter <im.alexcarpenter@gmail.com>
4 participants
Description
This PR contains a complete implementation of the
appearanceprop for the new AIO components. Notable changes from the existingappearanceprop include the dropping of arbitrary CSS (such as pseudo elements) and the removal of component-specific styles likeappearance={{ elements: { signIn: { ... } } }}.This PR also contains the following sub-PRs:
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change