Skip to content
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

[DO NOT MERGE] Spike: USWDS experiment #1003

Closed
wants to merge 1 commit into from
Closed

Conversation

dzole0311
Copy link
Collaborator

Related Ticket: {link related ticket here}

Description of Changes

{Update with description of changes made in this pull request}

Notes & Questions About Changes

{Add additonal notes and outstanding questions here related to changes in this pull request}

Validation / Testing

{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}

Copy link

netlify bot commented Jun 17, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 04597a1
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6682fdcf37ad85000867ee2c
😎 Deploy Preview https://deploy-preview-1003--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hanbyul-here hanbyul-here force-pushed the 959-uswds-experiment branch from 5939b37 to 04597a1 Compare July 1, 2024 19:04
@hanbyul-here
Copy link
Collaborator

👋 Re: css tree shaking - I am not sure if I am missing anything, but Parcel documentation says only named imports can take advantage of tree shaking: https://parceljs.org/languages/css/#tree-shaking . (I do not know how I can use named imports with scss? Is this even doable?)

Considering this issue is still opened in uswds - uswds/uswds#5210 I wonder if it is not doable with uswds style at this point? - If so, it might be beneficial to build css without any hash for now so it gets cached?

@dzole0311
Copy link
Collaborator Author

dzole0311 commented Jul 2, 2024

👋 Re: css tree shaking - I am not sure if I am missing anything, but Parcel documentation says only named imports can take advantage of tree shaking: https://parceljs.org/languages/css/#tree-shaking . (I do not know how I can use named imports with scss? Is this even doable?)

Yeah, I had the same problem which I mentioned in the spike doc. I think if there were uswds component mixins (ie. 'card'), we could have found a way to use the named import in Parcel but IMO on the expense of the dev UX:

// card.module.scss
.card {
    @include card <- mixin that doesn't exist in uswds
}

// card.tsx

import * as CardStyle from './card.module.scss'

<SomeCard className={CardStyle.card} />

Considering this issue is still opened in uswds - uswds/uswds#5210 I wonder if it is not doable with uswds style at this point? - If so, it might be beneficial to build css without any hash for now so it gets cached?

Fully hashless? Not sure how caching is set up in the instances but we have to assess potential cache invalidation issues for some clients browsers. Thinking of which, each hash is generated per veda instance release, so as long as it stays the same it should be cached across soft reloads of the page I think 🤔

@hanbyul-here
Copy link
Collaborator

@dzole0311 Ah, yes, you did mention it in the Spike doc. I'm sorry I blanked on it!

I meant detaching hash only for USWDS CSS since the CSS file content won't change if we include every style from USWDS—all other assets should still be hashed. We will likely need to tweak how assets are built for this - as far as I understand, we build assets per page, and any imported asset gets included into ${page}.hash.css. We would need to separate uswds css from other assets on the page if we want the hashless cache strategy to work 🤔

I still want to see if there are any more options to purge css afterward, such as https://www.npmjs.com/package/purgecss.

@dzole0311
Copy link
Collaborator Author

Work continues in 1029

@dzole0311 dzole0311 closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants