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

Import tailwind #783

Merged
merged 4 commits into from
Oct 7, 2020
Merged

Import tailwind #783

merged 4 commits into from
Oct 7, 2020

Conversation

brdunfield
Copy link
Contributor

@brdunfield brdunfield commented Oct 6, 2020

This PR imports the work done on https://github.com/cds-snc/notification-homepage into this PR.

https://trello.com/c/XF2KTVt0/51-merge-home-page-into-notification-admin-repo
cds-snc/notification-api#1134

I have included the necessary npm dev dependencies from notification-homepage, and included / adjusted the relevant config files. style.css is a direct copy from notification-homepage, as are the postcss and tailwind config files.

The resulting homepage.css is larger than what master outputs, likely because purgecss is scanning all html files, instead of just the homepage.

This is probably good enough for now. We can adjust this config if / when we decide to start splitting stylesheets.

NPM is complaining about several outdated dependencies, but in order to keep things clean and not cause issues with Antoine's dependency bump PR I have left them as they are.

Note: Running npm run webpack also generates a large amount of changes in main.min.js as a result of a moment.js dependency upgrade. We don't want these changes, as they are currently breaking, so we'll need to make sure they are not committed or included in npm run build as style updates are made in the future. I've logged this as an issue to fix here: cds-snc/notification-api#1150

@maxneuvians maxneuvians temporarily deployed to notification-import-tai-lgfzrg October 6, 2020 16:26 Inactive
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 6, 2020

This pull request introduces 1 alert when merging 79ab32d into 9c198d4 - view on LGTM.com

new alerts:

  • 1 for Overwritten property

@brdunfield brdunfield temporarily deployed to notification-import-tai-lgfzrg October 6, 2020 18:08 Inactive
@brdunfield brdunfield marked this pull request as ready for review October 6, 2020 18:11
Copy link
Contributor

@AntoineAugusti AntoineAugusti left a comment

Choose a reason for hiding this comment

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

Good work! 👏

I think you can update your PR description to reflect that there are no breaking changes for moment as we discovered yesterday.

I'm surprised there are no HTML files in your PR, are they already in this repo?

@@ -0,0 +1,27 @@
const cssnano = require("cssnano");

console.log("=== POST CSS ===");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove this

@@ -0,0 +1,154 @@
@tailwind base;
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this file is a straight copy/paste for now. Do you plan to change it later? I think it's not Tailwind's spirit to add custom classes like feature-box, but instead, rely on utilities as much as possible.

It would be a good idea to extract the CSS where we apply classes to a different CSS file as well.

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, my next task is to figure out how we want to split up the CSS, do components, etc, and adjust the source / target input / outputs accordingly.

@brdunfield
Copy link
Contributor Author

The HTML is already in this repo, yes. It's just the css. The markup generated by notification-homepage has been copied here previously, and no changes have been made since then. Now that the css build is happening locally, we should be able to modify the markup within this repo and see the css changes reflected in the css here.

@AntoineAugusti
Copy link
Contributor

Awesome, thanks for the clarification!

:shipit:

@brdunfield brdunfield merged commit 73498a7 into master Oct 7, 2020
@brdunfield brdunfield deleted the import-tailwind branch October 7, 2020 13:58
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