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

feat(minify shadowroots): add rollup-plugin-minify-html-literals #11207

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

marcelojcs
Copy link
Contributor

@marcelojcs marcelojcs commented Dec 7, 2023

Related Ticket(s)

Jira ticket

Description

Adds rollup-plugin-minify-html-literals to the build pipeline. This minifies the HTML resulting from each component's render() function.

Changelog

Changed

  • Added rollup-plugin-minify-html-literals to rollup config

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 7, 2023

@andy-blum
Copy link
Member

There's a change needed to the table-of-contents component. See here

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 13, 2023

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 13, 2023

@andy-blum
Copy link
Member

@marcelojcs make sure to run yarn ci-check from the project root. It's pointing out the errors in the ci-check job.

carbon-for-ibm-dotcom/packages/carbon-web-components/src/components/checkbox/checkbox-story.ts
Warning:   13:10  warning  'ifDefined' is defined but never used  @typescript-eslint/no-unused-vars

@m4olivei m4olivei added the test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing label Dec 18, 2023
@m4olivei m4olivei closed this Dec 18, 2023
@m4olivei m4olivei reopened this Dec 18, 2023
Copy link
Contributor

@m4olivei m4olivei left a comment

Choose a reason for hiding this comment

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

I added the test: CDN preview tag and closed / reopened to get that build triggered so we can compare bundle sizes.

Otherwise, just minor notes from a static code review.

Will come back and look at the build results when ready.

packages/web-components/package.json Outdated Show resolved Hide resolved
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 18, 2023

@m4olivei
Copy link
Contributor

@andy-blum
Copy link
Member

The impact of this change likely won't reflect in bundle sizes, but should appear in total node counts.

Card Group with Card-In-Card demos:

Current
Screenshot 2023-12-19 at 9 53 43 AM

This PR
Screenshot 2023-12-19 at 9 54 10 AM

@andy-blum
Copy link
Member

Note that the above comment is only testing the rollup changes which affect the CDN assets, not the gulp changes that impact the npm packages.

@andy-blum andy-blum self-requested a review January 9, 2024 14:56
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jan 19, 2024

@@ -70,8 +70,10 @@
"@babel/runtime": "^7.16.3",
"@carbon/styles": "1.47.0",
"flatpickr": "4.6.1",
"gulp-minify-html-literals": "^1.1.22",
Copy link
Member

Choose a reason for hiding this comment

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

This package doesn't seem to exist. Is this the one you meant to include?

@kennylam
Copy link
Member

kennylam commented Jan 19, 2024

Also, is this PR a duplicate of #11207?

@m4olivei
Copy link
Contributor

Also, is this PR a duplicate of #11207?

This PR is #11207. Do you mean #11216? #11216 is against the v1 branch, wheras this one is against main, I believe that's the only difference, but @marcelojcs and/or @bruno-amorim can correct me if I'm wrong.

@kennylam
Copy link
Member

kennylam commented Jan 20, 2024

Do you mean #11216? #11216 is against the v1 branch

Oops, pasted the wrong link. Yep I was referring to 11216, but totally missed the different target branches. Got it now, thanks!

@andy-blum
Copy link
Member

The gulp-minify-html-literals package was from here. I'm not sure why that package no longer exists, but we can remove it from this PR.

This PR is also on hold, pending the project's update to Lit v3 because that version seems to have scooped up the underlying minify-html-literals project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants