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

Remove header.css and replace with tailwind styles #784

Merged
merged 5 commits into from
Oct 9, 2020
Merged

Conversation

brdunfield
Copy link
Contributor

Towards:
cds-snc/notification-api#1137
https://trello.com/c/davC2pOu/66-translating-existing-styles-to-tailwind

I think the best way to approach this is piece by piece, and remove one existing css file at a time.

To start, I removed header.css, which mostly affects main_template.html, and a few buttons (and by extension page_footer.html). The homepage, header, footer, and most of the buttons in Notify should now be running off of Tailwind.

I have scaffolded out a few files for managing the CSS, but I expect that this will iterate and be built on / changed around as the process continues. Typography.css is currently empty. I'll either fill it or delete it in the next PR / iteration. Any feedback here on ideas of how to structure this would be great. I probably will not make too many changes in this PR unless there's a necessary reason for it, but would be happy to implement them in the next PR / iteration.

The next PR I think will be to continue the button work -> buttons.scss is my next target ⚔️ .

Notes on how the process works:

  1. New CSS is being written in files inside /app/assets/stylesheets/tailwind.
  2. When npm run webpack is run, these files are compiled into homepage.css inside /app/assets/stylesheets. Eventually I will rename this file, likely in the next iteration. Note: In this stage, webpack makes a lot of changes to main.min.js, that we need to manually discard for now before running the build step. This is partially tracked here, but we can probably make a separate card for diagnosing / fixing this.
  3. Then, when npm run build is run, the compiled files are copied into /app/static where they are served to the browser

@maxneuvians maxneuvians temporarily deployed to notification-organize-c-9m9ljr October 8, 2020 19:35 Inactive
@brdunfield brdunfield temporarily deployed to notification-organize-c-9m9ljr October 8, 2020 19:36 Inactive
@@ -24,22 +26,36 @@ li {
@apply text-base leading-tight;
}

.btn {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will probably split out buttons and other form elements into another file in the next PR

@brdunfield brdunfield temporarily deployed to notification-organize-c-9m9ljr October 8, 2020 19:43 Inactive
@@ -13,7 +15,7 @@ h2 {
}

a {
@apply underline text-base;
@apply underline text-smaller;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the old css file had a smaller size for anchors - without this change, all the links on pages outside of signedout.html got larger and no longer matched the rest of the paragraph text.

@@ -0,0 +1,4 @@
@layer components {
/* links */
Copy link
Contributor

Choose a reason for hiding this comment

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

Typography instead of links?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure what will end up in this file. I named it typography so that it could hold things like headers, paragraphs, links. But I haven't touched any headers or paragraphs yet.

@@ -11,9 +11,10 @@
<link
rel="stylesheet"
media="screen"
href="{{ asset_path }}stylesheets/header.css?0.24.2"
href="{{ asset_path }}stylesheets/homepage.css"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth renaming this CSS file now? I think it'll be our future only file. And now that it's included everywhere, maybe we need to reflect this, or a bit later.

There was a version before, do we need a hash? How does caching work for static assets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea what the version was there for before, I didn't see this number referenced anywhere else in the original file.

I can rename homepage.css now if you like, I was planning to do so in the next PR anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it was there for cache-busting. We need to make sure that assets are properly cached because we're going to do multiple updates on this file over the coming weeks and we don't want people to use their local version because the filename is the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Renaming later is fine, seems like you have a good plan in your head already, awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this then.

The fix, or at least what we've been doing so far, is to just run an npm build after pulling master. However, that's not something we have to do all the time so unless it's communicated to all the devs to do so on their next pull it can easily be forgotten.

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't want people to use their local version because the filename is the same.

I had in mind visitors to the website, where their browser would use a previous version of this file because of cache headers. I think that the dev setup is fine 👌

As we discussed on Slack, seems like these static assets are never cached and are downloaded on every request. This is convenient for developers but not an ideal experience for visitors as we force a lot of unneeded downloads.

@brdunfield brdunfield temporarily deployed to notification-organize-c-9m9ljr October 9, 2020 13:30 Inactive
@brdunfield brdunfield merged commit fc3a491 into master Oct 9, 2020
@brdunfield brdunfield deleted the organize-css branch October 9, 2020 14:20
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.

3 participants