-
Notifications
You must be signed in to change notification settings - Fork 246
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
feat(clerk-js): Add support for loading UI styles as first CSS stylesheet #4291
feat(clerk-js): Add support for loading UI styles as first CSS stylesheet #4291
Conversation
|
import { ClerkInstanceContext, OptionsContext } from '@clerk/shared/react'; | ||
import type { ClerkHostRouter } from '@clerk/shared/router'; | ||
import { ClerkHostRouterContext } from '@clerk/shared/router'; | ||
import type { ClerkOptions, LoadedClerk } from '@clerk/types'; | ||
//@ts-ignore - This is treated as a string export by Webpack |
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.
[question] Would it be better to use@ts-expect-error
so in the case this is no longer an issue in the future the @ts-ignore
doesn't just stick around forever?
packages/clerk-js/global.d.ts
Outdated
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.
Deleted this file as it wasn't being included anyway, only the src/
directory was.
@@ -43,6 +41,12 @@ export function init({ wrapper }: { wrapper: ElementType }) { | |||
rootElement = document.createElement('div'); | |||
rootElement.setAttribute('id', 'clerk-components'); | |||
document.body.appendChild(rootElement); | |||
|
|||
const stylesheet = document.createElement('link'); |
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.
@BRKalow I'm assuming since we're within an instance where the rootElement
didn't exist that we can safely assume the stylesheet isn't part of the DOM. Do you think we need to add some attributes to the link
element and check for those?
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.
@dstaley It should be safe, but in this case I feel like this is a place where we it would be a good idea to add the check.
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.
added! 4380da2
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.
👏
Description
This PR adds support for inserting the new UI stylesheet as the first stylesheet in the document. This is to allow user styles to take precedence over our own. For example:
If the UI stylesheet appears after the app stylesheet in the DOM,
cl-button
will overridehidden
given they have the same specificity. By prepending our stylesheet, we allow our users' styles to take precedence.To support this, I had to remove Webpack's experimental support for CSS which doesn't currently support emitting the URL of the stylesheet; it always wants to append it to the DOM.
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change