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

Bundle CSS into fides.js script, and add styles to DOM from the package #3288

Closed
eastandwestwind opened this issue May 11, 2023 · 6 comments
Closed
Assignees

Comments

@eastandwestwind
Copy link
Contributor

eastandwestwind commented May 11, 2023

Current behavior

Currently, rollup bundles the CSS into a separate output in fides-js/dist/fides.css.

For usage in privacy-center/public/fides-js-components-demo.html, we have to copy the CSS file over into privacy-center/public/lib/fides.css so that the page can import it directly in the HTML.

For usage in privacy-center/public/fides-js-demo.html file, we dynamically insert the css from fides-js/dist/fides.css, just like we do the js file from fides-js/dist/fides.js. This, specifically, is error-prone.

Expected behavior

We should bundle the CSS into fides-js/dist/fides.js itself so that we won't need to insert the css at the last minute in the bundle on the privacy center.

Proposed Approach

  1. Create a declaration.d.ts file with declare module "*.module.css"; in fides-js/src
  2. Add the below to ConsentBanner.tsx:
import styles from "../lib/banner.module.css" assert { type: "css" };

if (typeof window !== 'undefined') {
  // @ts-ignore
  // Create an empty "constructed" stylesheet
    const sheet = new CSSStyleSheet();
  // Apply a rule to the sheet
  sheet.replaceSync(JSON.stringify(styles));
  document.adoptedStyleSheets = [sheet];
}
  1. Remove the extra css imports from privacy-center/public/fides-js-components-demo.html and privacy-center/pages/api/fides-js.ts

This currently does not add the styles appropriately, so will need investigation.

Context

Originally posted by @NevilleS in #3191 (comment)

@NevilleS
Copy link
Contributor

Yeah- this one needs to land before the "final" release but can wait for the all the components to be working first 👍

@NevilleS
Copy link
Contributor

NevilleS commented Jun 1, 2023

I'm pretty sure this CSS bundling is why this is happening in my testing:

Image

😬

I think it's just very unreliable to write that CSS to the document right now.

@NevilleS
Copy link
Contributor

NevilleS commented Jun 1, 2023

I've done some research in the past on Rollup plugins for CSS, so here's a quick roundup of options.

I like this community-maintained list of plugins: https://github.com/rollup/awesome

From here we want a CSS plugin that hits a couple requirements:

  • allows an easy import statement in the JSX files
  • injects the CSS into the bundle
  • handles adding the CSS into the DOM, ideally with a nice standalone stylesheet
  • importantly doesn't mangle the class names, since we want it to be easy for a customer to override the styles

Will need to test a few out to be sure, but based strictly on download popularity as a really rough metric (sorry, OSS maintainers!) here's how the list stacks up:
image
(results here)

...it's not even close, rollup-plugin-postcss just dominates the download charts. It doesn't seem to get any regular updates though: https://github.com/egoist/rollup-plugin-postcss hasn't seen a release for 2 years, but every search result I look for related to Rollup and CSS leads me to that project, which seems to do what we need.

In the herd of much-less-popular alternatives, this one strikes me as the best option: https://github.com/Anidetrix/rollup-plugin-styles. It is more recently maintained and still is pretty popular as far as npm packages go:
image

So I'd plan to just compare those two plugins and see what I find.

@NevilleS
Copy link
Contributor

NevilleS commented Jun 1, 2023

rollup-plugin-postcss worked nicely: #3431

I took a closer look at https://github.com/Anidetrix/rollup-plugin-styles and, while it's more recently updated, it's also a bit stale and looks like it's fallen out of compatibility with rollup v3 and folks are forking it to maintain it, etc. So I think, unfortunately, the metric of "what has the most weekly downloads?" proves effective yet again...!

@NevilleS NevilleS assigned NevilleS and unassigned eastandwestwind Jun 2, 2023
@NevilleS
Copy link
Contributor

NevilleS commented Jun 4, 2023

Closed by #3431

@NevilleS NevilleS closed this as completed Jun 4, 2023
@Roger-Ethyca
Copy link
Contributor

moving to done

Image

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

No branches or pull requests

3 participants