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

fix: postinstall of css artifact #149

Merged
merged 2 commits into from
Feb 16, 2021
Merged

fix: postinstall of css artifact #149

merged 2 commits into from
Feb 16, 2021

Conversation

derberg
Copy link
Member

@derberg derberg commented Feb 15, 2021

Description

Atm artifacts are not generated during release and not pushed to npm. This PR fixes it and introduces a separate css file that is considered to be generated-only

@chatne thanks for spotting the issue

  • introduce main.min.css that will always be .gitignore and will be a generated artifact.
  • add postinstall script that always generates fresh main.min.css file
  • use only main.min.css in the HTML, no matter the mode (singleFile or not)
  • main.min.css will not be pushed to npm as it will be anyway always generated after installation + it will never be updated in the repository

@Theiaz @marcortw what do you think about this change?

Related issue(s)
See also #142 (comment)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@marcortw
Copy link

That sounds reasonable to me, thanks @derberg!

Comment on lines +28 to +31
"generate:atomcss": "cross-env NODE_ENV=production postcss template/css/atom-one-dark.min.css -o template/css/main.min.css",
"generate:maincss": "cross-env NODE_ENV=production postcss template/css/custom.css -o template/css/main.min.css",
"generate:tailwind.css": "cross-env NODE_ENV=production postcss template/css/tailwind.min.css -o template/css/main.min.css",
"postinstall": "rimraf \"template/css/main.min.css\" && npm run generate:atomcss && npm run generate:maincss && npm run generate:tailwind.css",
Copy link
Member

Choose a reason for hiding this comment

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

I have one question. Does postcss merge everything to one main.min.css, yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

define everything 😄 there is no way to merge multiple files into one at once, nothing in the CLI that I've seen 😞

Copy link
Member

@magicmatatjahu magicmatatjahu Feb 16, 2021

Choose a reason for hiding this comment

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

So, postinstall script overrides main.min.css 3 times, or what? 😄 EDIT: Or only appends new content?

Copy link
Member Author

Choose a reason for hiding this comment

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

appends/merges

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean not a simple append but smart merge 😄

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM

@derberg derberg merged commit c232c70 into asyncapi:master Feb 16, 2021
@derberg derberg deleted the maincss branch February 16, 2021 07:50
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.17.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Theiaz
Copy link
Contributor

Theiaz commented Feb 19, 2021

Sorry for being late.

I had a look on the changes and I want to leave my opinion:

  • introduce main.min.css that will always be .gitignore and will be a generated artifact.

As far as i can see, that changes has already been reverted in #153

  • add postinstall script that always generates fresh main.min.css file

Merging all css files into one is a good idea. However you do not need to do it this way. There are easier options like @import. Postcss can handle this and its working fine with tailwind. We just needs to use postcss-import and adjust our file structure:

We just need two files: atom-one-dark.css and main.css with the following structure:

@import "./atom-one-dark.css"; 

@tailwind base;

@tailwind components;

@tailwind utilities;

/* our custom css */
html,
body { ... }

....

I've tested it locally and will open a PR.

  • use only main.min.css in the HTML, no matter the mode (singleFile or not)

Thats fine!

  • main.min.css will not be pushed to npm as it will be anyway always generated after installation + it will never be updated in the repository

What about separation of development files and generated assets / build files? Would it be possible to place our "output" files like main.min.css inside a seperate dist folder or do they really needs to be inside template/? Possibly this can be extended to .js and .html files aswell? There would be no need to push dist folder anymore and generated assets could easily be dropped.

@derberg
Copy link
Member Author

derberg commented Feb 19, 2021

@Theiaz hey man, no worries, thanks for getting back with valuable input and a PR!

template dir is a must as the generator only touches files that are there in the template, we can have template/dist of course, and at the end dist goes to the output

@derberg
Copy link
Member Author

derberg commented Feb 19, 2021

generator support hooks into the generation process, we can have a hook that after generation removes not needed files from the output. We could actually move postcss processing to hooks too if this is needed and if possible to use postcss as a library. Here you have an example of the hook https://github.com/asyncapi/html-template/blob/master/hooks/02_renameOutFile.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants