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.

Sign up for GitHub

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

Jump to bottom

CDN install instructions #919

Merged
merged 21 commits into from
Aug 8, 2022
Merged

CDN install instructions #919

merged 21 commits into from
Aug 8, 2022

Conversation

xjensen
Copy link
Collaborator

@xjensen xjensen commented Aug 1, 2022 •
edited
Loading

Per #324, updates component documentation with CDN installation instructions.

Also includes #923 by adding dist folders for all CSS-only components. Link-icon also gets a dist folder, and it's CSS is now inserted via JS to bring it into consistency with the other JavaScript components.

@xjensen
Copy link
Collaborator Author

xjensen commented Aug 1, 2022

Thus far, I've only updated the accordion documentation. If this looks good, let me know and I'll go through and do the rest.

@github-actions
Copy link

github-actions bot commented Aug 1, 2022

Preview site available at cdn-install-instructions.pr.designsystem.webstandards.ca.gov.

@xjensen xjensen mentioned this pull request Aug 1, 2022
Closed
@xjensen xjensen requested review from aaronhans and zakiya August 1, 2022 20:43
aaronhans
aaronhans approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@aaronhans aaronhans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good

@zakiya
Copy link
Collaborator

zakiya commented Aug 1, 2022

See my comment at #324

@xjensen
Copy link
Collaborator Author

xjensen commented Aug 4, 2022 •
edited
Loading

I've added CDN installation instructions for a majority of the components.

Some components have been excluded due to inconsistent file paths for CSS/JS, which has led to broken CDN uploads. #923 describes this problem. I think we'll need to work through that issue to fully finish this PR.

Still to-do:

  • icons (CDN uploader needs to support SVG)
  • highlight-section
  • base-css
  • combined-css
  • link-icon
  • table
  • regulatory-outline

@xjensen xjensen mentioned this pull request Aug 5, 2022
Closed
@xjensen
Copy link
Collaborator Author

xjensen commented Aug 5, 2022

#923 has been added to this PR.

  • All components now have a dist folder, including CSS-only components.
  • Major version numbers have been bumped in affected components.
  • Link-icon gets a dist folder too. It's been modified to insert CSS via JS like the other JavaScript components.
  • The CDN publisher has been altered to support these changes. It will now publish CSS to the dist folders.

I did not add CDN installation instructions to base-css and combined-css. We need to do a lot more with those READMEs to make them useful, so I'm punting.

I also skipped icons. I suspect there may be a larger discussion worth having about how to make that available via CDN.

Finally, I feel we should remove the component dist folders from the Git repo. They are build artifacts. We would still include them in the NPM packages, of course. But that's a project for another day.

@zakiya
Copy link
Collaborator

zakiya commented Aug 5, 2022

All components now have a dist folder, including CSS-only components.

This sparks joy. ✨

@xjensen xjensen requested a review from aaronhans August 5, 2022 22:57
@xjensen xjensen changed the base branch from main to release/1.0.10-fremont-peak August 8, 2022 15:51
@xjensen
Copy link
Collaborator Author

xjensen commented Aug 8, 2022

@aaronhans @zakiya I'd like to get a fresh review on this PR if y'all don't mind. It's changed quite a bit since the previous approval, including major version bumps to half the components.

aaronhans
aaronhans reviewed Aug 8, 2022
View reviewed changes
docs/src/publish/cdn-deploy.js
if (filePath.indexOf('.css') > -1) {
key = cssKey;
}
const key = keyPrefix + filePath.split(`${dir}/`)[1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

aaronhans
aaronhans approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@aaronhans aaronhans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @xjensen! Love all the major version bumps and standardization we have now

@xjensen
Copy link
Collaborator Author

xjensen commented Aug 8, 2022

I'm going to merge into release branch and publish to NPM. The major version bumps should keep all our sites safe.

@xjensen xjensen merged commit b320f54 into release/1.0.10-fremont-peak Aug 8, 2022
@xjensen xjensen deleted the cdn-install-instructions branch August 8, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronhans aaronhans aaronhans approved these changes

@zakiya zakiya Awaiting requested review from zakiya

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xjensen @zakiya @aaronhans