Replace TailwindCSS with Protocol and SCSS for styling #312

Iinh · 2021-01-11T16:38:26Z

Continuation from the work started in PR#294 to transition to Protocol from Tailwindcss.

Add SCSS support for Svelte
Add SCSS support for Storybook
Use Protocol: right now only what's needed from the package is copied over. I had to copy the files instead of importing for customization purposes, such as replacing the type scale setting from regular to condensed.

Since the goal was to remove Tailwind, my focus was less on making sure the transformation looks pixel perfect. There are sections where styling still looks awkward, I'd appreciate the feedback on this front.

Work to be done in follow up PRs:

Improve header from its current minimal state
Abstract the logic used for App/Metric/Ping/TableDetails description table to a separate component. Right now styling is repeated in these 4 components.

Pull Request checklist

The pull request has a descriptive title (and a reference to an issue it
fixes, if applicable)
All tests and linter checks are passing
The pull request is free of merge conflicts

.storybook/preview.js

wlach

This looks great!

My main feedback at this point is that I think some files shouldn't be copied over into the repo. In particular, I believe we should have everything we need (after running npm install) in node_modules/@mozilla-protocol. For the fonts, I believe we should be able to get rollup to copy them over from there into public/.

Let me know if I'm missing something.

wlach · 2021-01-11T16:53:39Z

.storybook/main.js

+          style: "scss",
+        },
+        scss: {
+          prependData: `@import 'src/scss/protocol/includes/_lib.scss';`,


Suggested change

prependData: `@import 'src/scss/protocol/includes/_lib.scss';`,

prependData: `@import 'node_modules/@mozilla-protocol/core/protocol/css/includes/_lib.scss/scss/protocol/includes/_lib.scss';`,

The type scale, which needed to be changed from the default "standard" to "condensed", is defined in /includes/lib, which then gets the value for sizing from /includes/themes. I could just directly update this in node_modules/@mozilla-protocol, but since node_modules is gitignored, should I remove all the copied scss files, make the edit directly in the package, and un-gitignore node_modules/@mozilla-protocol?

Oh I see, no definitely don't un-gitignore anything in node_modules/*! That's meant to be installed dynamically at build time.

I think it would be better if we copied as little as possible from protocol into this repository, and reuse as much as possible as-is from what's installed in node_modules/. That way we can just upgrade the protocol dependency as we go to gain new features and fixes as they come in, with hopefully a minimal number of changes inside this repository.

However, I admit that I've never done anything like this before (integrating a sass library into a repository) so there might be something I'm missing. I wonder if @craigcook might be able to give us some guidance on how best to handle this (whether that be copying parts of protocol into glean dictionary, or something else...)

Yeah right now only what's needed is copied over, which is the /base folder and a couple files from /components. The reason why the number of migrated files is big is because I can't just pick and choose the component files, I have to also copy the dependencies which are the scss variables and mixins in /includes. Not sure if it's necessary, but I did add a README that explains the process to minimize confusion for other contributors.

I'm not sure I see a reason you couldn't copy the whole Protocol framework into your project rather than referencing node_modules directly, though I'm not sure how you'd actually do the copying in your particular setup (I've only done it with Gulp). Since you seem to be copying over some of it already you might as well get whole thing.

And because you're now compiling Sass yourself you can definitely pick-and-choose specific components at the compile step. The protocol-components.css file is only provided for use if you're not compiling your own Sass, but it ends up being a huge file with EVERY component so it will always be more than you need. You don't need it at all if you're compiling yourself, just @import the components you're actually using.

What we've done on other sites is copy Protocol from node_modules into some local build folder and all our own SCSS is in a local scss folder (or whatever you want to name it). The local SCSS can reference the local copy of Protocol files, and any customization or extensions we need to make to it are only done in our own SCSS (relying on specificity or the cascade to override the original Protocol styling). Definitely don't change any of the Protocol source files since any changes would just be overwritten next time you copy it.

You can also configure the brand theme and type scale in your local SCSS.

What we've done on other sites is copy Protocol from node_modules into some local build folder and all our own SCSS is in a local scss folder (or whatever you want to name it). The local SCSS can reference the local copy of Protocol files, and any customization or extensions we need to make to it are only done in our own SCSS (relying on specificity or the cascade to override the original Protocol styling). Definitely don't change any of the Protocol source files since any changes would just be overwritten next time you copy it.

Is there any reason you'd recommend copying protocol into the project and checking it in, vs. just referencing it via node_modules? The latter feels a little more elegant to me.

Is there any reason you'd recommend copying protocol into the project and checking it in, vs. just referencing it via node_modules? The latter feels a little more elegant to me.

I suppose not, other than shortening the paths on your imports. I just meant to say you can copy it without hurting anything as long as you're not modifying anything.

That's just what we've done on other sites like bedrock or kitsune, which admittedly have a lot more stuff happening during the build process so there might be performance advantages to a local copy in those cases that might not apply to you. And we only copy it into a local build folder; the copy isn't committed to git.

Some things you will want to copy and commit, such as the fonts, icons, and any scripts you're using from Protocol. Anything that needs to be served to the client, but you could do that manually. All the Protocol SCSS could stay in node_modules if that's better for you.

src/__snapshots__/storyshots.test.js.snap

wlach · 2021-01-11T21:05:28Z

Was playing around with the site a bit this afternoon. I think the styling mostly looks really good. There's just two things that stick out:

The breadcrumb widget could use some more padding.

This is a little subjective, but I find the font sizes a little small and hard to read on my screen. I think the defaults for standard text that were in the initial PR were better.

Iinh · 2021-01-11T23:17:31Z

This is a little subjective, but I find the font sizes a little small and hard to read on my screen. I think the defaults for standard text that were in the initial PR were better.

The reduction in font size was due to the change in type scale to "condensed". If your preference is go back to the standard scale, and the only place that we want condensed typography is the h1 tags, would it be better that we go with your original idea of creating a separate component for it? This will also eliminate the need for theme customization (for now) and let us directly import from node_modules/.

* Use Protocol styling for footer * Remove extra class name * Update footer license info

wlach

This is looking quite excellent, thank you @Iinh! Two items:

I think we can get around needing to import the fonts into public/ by adding the following symbolic links:

node_modules/@mozilla-protocol/core/protocol/img -> public/img
node_modules/@mozilla-protocol/core/protocol/fonts -> public/fonts

Can you give that a try? You should be able to make the links like so from the root directory:

ln -s node_modules/@mozilla-protocol/core/protocol/img public/img

(then just add them to the PR)

It seems like storybook building is timing out on netlify, do you have any idea why? I turned it off for now, since the last build took 20 minutes (!) Here's a snapshot of the log, which was 250k+ lines long: https://gist.github.com/wlach/bb799b808500e8ddd9c390907a143ddb

I am personally ok with fixing the second item later (since I only recently added the storybook deploy to the netlify deploy preview, and wasn't really depending on it) but if it's easy to figure out...

src/components/Footer.svelte

src/components/SchemaViewer.svelte

.eslintrc.js

.prettierignore

package.json

src/components/Pill.svelte

src/components/Title.svelte

src/components/icons/InfoIcon.svelte

src/pages/AppDetail.svelte

svelte.config.js

Iinh · 2021-01-13T07:04:54Z

It seems like storybook building is timing out on netlify, do you have any idea why? I turned it off for now, since the last build took 20 minutes (!) Here's a snapshot of the log, which was 250k+ lines long: https://gist.github.com/wlach/bb799b808500e8ddd9c390907a143ddb

Because we imported the whole Protocol package but are only using some of it, there's a lot of unused CSS selectors, and svelte-loader doesn't like this. From reading the build log, I think the reason why it took so long to deploy was because Netlify logged the warnings for every single unused CSS selector, which understandably took a long time since there are hundreds of them.

My initial approach to solve this (this commit) was to find a way to remove the unused CSS selectors using postcss-purgecss -- the library that we used with Tailwind. It did remove the majority of the unused CSS, but not all of them. It also threw off the styling, probably because it might have removed some of the global styles.

Since what's causing the gridlock is the 250k+ lines of warning in Storybook, my next approach was to turn off these warnings completely in the svelte-loader setting inside Storybook. I think doing this is safe because this error is Storybook specific, I believe Svelte does not compile unused rules: sveltejs/svelte#697. So this was done in this commit. I think this will solve the issue, although I'm not 100% sure. Is it possible to add me to the Netlify team so I can check?

Other than that, I think I've addressed all of your requested changes. Let me know what you think @wlach.

wlach · 2021-01-13T14:28:17Z

It seems like storybook building is timing out on netlify, do you have any idea why? I turned it off for now, since the last build took 20 minutes (!) Here's a snapshot of the log, which was 250k+ lines long: https://gist.github.com/wlach/bb799b808500e8ddd9c390907a143ddb

...
Since what's causing the gridlock is the 250k+ lines of warning in Storybook, my next approach was to turn off these warnings completely in the svelte-loader setting inside Storybook. I think doing this is safe because this error is Storybook specific, I believe Svelte does not compile unused rules: sveltejs/svelte#697. So this was done in this commit. I think this will solve the issue, although I'm not 100% sure. Is it possible to add me to the Netlify team so I can check?

Great stuff @Iinh, thanks for such a thorough investigation. I just tried re-enabling the storybook build in netlify, and it now completes in a very reasonable 2 minutes and 18 seconds. 🚀 This seems like a very reasonable approach, I have some minor extra comments but this is looking 99% of the way done.

wlach

A few small things before we land (in addition to the review comments). Thanks, as always, for your patience and persistence:

npm run dev doesn't seem to work for me anymore, I'm getting a similar error to the one you were getting a few days ago: https://gist.github.com/wlach/8c7fc5abd046537f1d7348bfd8d55203
On reflection, I think we probably ought to use the colored Glean logo, if it looks at all reasonable-- I think it's probably more familiar to people, and more consistent with what's in the Glean documentation, etc. I was going to test this out myself, but got stuck on (1).

jest.config.js

public/fonts/fonts

src/components/SchemaViewer.svelte

.storybook/main.js

Iinh · 2021-01-13T17:10:58Z

npm run dev doesn't seem to work for me anymore, I'm getting a similar error to the one you were getting a few days ago: https://gist.github.com/wlach/8c7fc5abd046537f1d7348bfd8d55203

I only get this error if I compile SCSS with sass. Do you still have sass in your package.json? If yes you need to remove it and install npm i node-sass instead (so svelte-preprocess doesn't use sass as default over node-sass)

On reflection, I think we probably ought to use the colored Glean logo, if it looks at all reasonable

Done! I actually like this header better. Thank you for suggesting this improvement.

vs

One last thing, I was trying to remove the symbolic links created, but I'm not very familiar with this so could you help check if the links have been removed? Thank you! @wlach

wlach · 2021-01-13T18:54:19Z

npm run dev doesn't seem to work for me anymore, I'm getting a similar error to the one you were getting a few days ago: https://gist.github.com/wlach/8c7fc5abd046537f1d7348bfd8d55203

I only get this error if I compile SCSS with sass. Do you still have sass in your package.json? If yes you need to remove it and install npm i node-sass instead (so svelte-preprocess doesn't use sass as default over node-sass)

Just to be sure, I tried this with a fresh checkout on your branch and am still getting the same error. Are you sure you checked everything in?

package.json

wlach

One last thing, I was trying to remove the symbolic links created, but I'm not very familiar with this so could you help check if the links have been removed? Thank you! @wlach

It looks like the links have been removed, but the files are still in your commit. You probably want to git rm -r public/fonts public/img and then make another commit.

Aside from that, just one more question. 😄

public/_redirects

wlach · 2021-01-13T21:17:27Z

This PR was 💯 -- congrats @Iinh! 🎉

Iinh · 2021-01-13T21:22:55Z

@wlach Thank you 😭 Filing a couple follow up issues from our discussion in this PR.

Iinh force-pushed the use-protocol-and-scss branch from 1b715e3 to 5e8a4c0 Compare January 11, 2021 16:42

Iinh requested a review from wlach January 11, 2021 16:43

Iinh commented Jan 11, 2021

View reviewed changes

.storybook/preview.js Show resolved Hide resolved

wlach suggested changes Jan 11, 2021

View reviewed changes

wlach and others added 7 commits January 11, 2021 21:33

WIP

3f10f64

Fix tests

28c11f3

Use Protocol for footer and fix schema viewer (#299)

ca25b38

* Use Protocol styling for footer * Remove extra class name * Update footer license info

Remove Tailwindcss configuration

edc2c9d

Replace Tailwindcss with SCSS and Protocol

67dde93

Update snapshot test & improve breadcrumb styling

57923a9

Style header

cf0a3b1

Iinh force-pushed the use-protocol-and-scss branch from ddc0ecf to 08ed0fc Compare January 12, 2021 06:20

Resolve dependency conflict

50f69fd

Iinh force-pushed the use-protocol-and-scss branch from 08ed0fc to 50f69fd Compare January 12, 2021 06:33

wlach suggested changes Jan 12, 2021

View reviewed changes

Iinh added 2 commits January 12, 2021 21:20

Remove unused CSS classes

ac21dad

Disable 'unused-css-selector' warning in Storybook

9934c1f

Iinh force-pushed the use-protocol-and-scss branch from d016404 to a853b8c Compare January 13, 2021 06:48

Iinh force-pushed the use-protocol-and-scss branch from a853b8c to 1e8b140 Compare January 13, 2021 14:13

Iinh marked this pull request as ready for review January 13, 2021 14:17

wlach suggested changes Jan 13, 2021

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

public/fonts/fonts Outdated Show resolved Hide resolved

src/components/SchemaViewer.svelte Show resolved Hide resolved

.storybook/main.js Outdated Show resolved Hide resolved

Iinh force-pushed the use-protocol-and-scss branch from 1e8b140 to e89a6bb Compare January 13, 2021 16:57

Iinh commented Jan 13, 2021

View reviewed changes

package.json Show resolved Hide resolved

wlach suggested changes Jan 13, 2021

View reviewed changes

public/_redirects Outdated Show resolved Hide resolved

Iinh force-pushed the use-protocol-and-scss branch 2 times, most recently from e171311 to 1a69e06 Compare January 13, 2021 21:01

Resolve requested changes

15b5df4

Iinh force-pushed the use-protocol-and-scss branch from 1a69e06 to 15b5df4 Compare January 13, 2021 21:05

wlach mentioned this pull request Jan 13, 2021

Initial attempt at using the protocol design system #294

Closed

7 tasks

wlach approved these changes Jan 13, 2021

View reviewed changes

wlach merged commit 6147269 into main Jan 13, 2021

wlach deleted the use-protocol-and-scss branch January 13, 2021 21:17

This was referenced Jan 14, 2021

Specify the dependencies in package.json #317

Closed

Add site favicon (fixes #154) #325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace TailwindCSS with Protocol and SCSS for styling #312

Replace TailwindCSS with Protocol and SCSS for styling #312

Iinh commented Jan 11, 2021 •

edited

Loading

wlach left a comment

wlach Jan 11, 2021

Iinh Jan 11, 2021 •

edited

Loading

wlach Jan 11, 2021

Iinh Jan 11, 2021

craigcook Jan 11, 2021

wlach Jan 11, 2021

craigcook Jan 11, 2021

wlach commented Jan 11, 2021

Iinh commented Jan 11, 2021

wlach left a comment

Iinh commented Jan 13, 2021

wlach commented Jan 13, 2021

wlach left a comment

Iinh commented Jan 13, 2021 •

edited

Loading

wlach commented Jan 13, 2021

wlach left a comment

wlach commented Jan 13, 2021

Iinh commented Jan 13, 2021

	prependData: `@import 'src/scss/protocol/includes/_lib.scss';`,
	prependData: `@import 'node_modules/@mozilla-protocol/core/protocol/css/includes/_lib.scss/scss/protocol/includes/_lib.scss';`,

Replace TailwindCSS with Protocol and SCSS for styling #312

Replace TailwindCSS with Protocol and SCSS for styling #312

Conversation

Iinh commented Jan 11, 2021 • edited Loading

Pull Request checklist

wlach left a comment

Choose a reason for hiding this comment

wlach Jan 11, 2021

Choose a reason for hiding this comment

Iinh Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

wlach Jan 11, 2021

Choose a reason for hiding this comment

Iinh Jan 11, 2021

Choose a reason for hiding this comment

craigcook Jan 11, 2021

Choose a reason for hiding this comment

wlach Jan 11, 2021

Choose a reason for hiding this comment

craigcook Jan 11, 2021

Choose a reason for hiding this comment

wlach commented Jan 11, 2021

Iinh commented Jan 11, 2021

wlach left a comment

Choose a reason for hiding this comment

Iinh commented Jan 13, 2021

wlach commented Jan 13, 2021

wlach left a comment

Choose a reason for hiding this comment

Iinh commented Jan 13, 2021 • edited Loading

wlach commented Jan 13, 2021

wlach left a comment

Choose a reason for hiding this comment

wlach commented Jan 13, 2021

Iinh commented Jan 13, 2021

Iinh commented Jan 11, 2021 •

edited

Loading

Iinh Jan 11, 2021 •

edited

Loading

Iinh commented Jan 13, 2021 •

edited

Loading