Use correct CSS version when injecting link tag #187
Conversation
razor-x
commented
May 27, 2023
razor-x
commented
- refactor: Prefer disabled over enabled
- refactor: Return early from effect
- docs: Import CSS in example
- feat: Add useUnminifiedCss prop
- fix: Use correct version for stylesheet
| endpoint={import.meta.env.SEAM_ENDPOINT} | ||
| publishableKey={import.meta.env.SEAM_PUBLISHABLE_KEY} | ||
| userIdentifierKey={import.meta.env.SEAM_USER_IDENTIFIER_KEY} | ||
| disableCssInjection |
There was a problem hiding this comment.
We have to put this in the example since when running locally the example should use the latest styles. We can keep the font injection.
| "lint": "eslint --ignore-path .gitignore --ext .js,.cjs,.mjs,.ts,.tsx .", | ||
| "prelint": "prettier --check --ignore-path .gitignore .", | ||
| "postlint": "stylelint '**/*.scss'", | ||
| "prepack": "echo \"export default '$(jq -r .version < package.json)'\" > lib/version.js", |
There was a problem hiding this comment.
Normally I would like to use a platform independent script (can always be done later), but this will ONLY run on CI so it should be safe to do this for now.
|
|
||
| export const useSeamStyles = ({ enabled }: { enabled: boolean }) => { | ||
| useEffect(() => { | ||
| if (version === null) return |
There was a problem hiding this comment.
In development the version will always be null.
razor-x
changed the title
|
UI test for the supported device table is flaky, seems it get stuck loading sometimes. |
| const cssUrl = 'https://www.unpkg.com/@seamapi/react@1.1.0/index.min.css' | ||
| import version from 'lib/version.js' | ||
|
|
||
| const origin = 'https://react.seam.co' |
There was a problem hiding this comment.
If we always use the css file in the release / bundle, wouldn't it always have the correct version?
There was a problem hiding this comment.
No because we are injecting a URL that contains the version number which must match the version of the package that is being used by the user. I'm not sure if that explanation is more clear, but it's correct.
There was a problem hiding this comment.
No because we are injecting a URL that contains the version number
Yep, was wondering what the benefits of injecting via URL vs. just including what is bundled with the package are?
There was a problem hiding this comment.
Ah, there is a pretty long internal discussion about this. The short version is that having it inject the fonts and the styles simplifies adoption and removes all the extra steps you need to use this.
It's still possible to opt out of the HTML injection and do it the other way for users who prefer that.
There was a problem hiding this comment.
Was actually thinking of dynamically importing the css via relative paths, but of course that wouldn't work because this is runtime.
What if we always include the CSS, but just change the class name? Won't have to deal with version numbers, and no dynamic loading necessary?
Root component / provider
const class = disableStyles ? '' : 'seam-components'
return (<div className={class}>...</div>)There was a problem hiding this comment.
What if we always include the CSS
I'm not sure I understand this. With this update we are always including the css unless the user opts out. I'd prefer not to inject the via an inline style tag since that bloats the js bundle size and cannot take advantage of caching the same way (among other things).
There was a problem hiding this comment.
I'd prefer not to inject the via an inline style tag
Yep, I mean we'd be able to avoid that. Here's a PR:
@seveibar It's a little difficult to test things that only happen in the built package (very few things that hit that edge case). It's possible though: the This case it pretty special though. We could add an example app that ONLY uses the deployed version (would not "sync" with local dev). That would give a way to test it (I basically did that locally to verify this will work). |
3 participants
