Skip to content

Fix licensing comment appearing thrice in main CSS file#164

Merged
jackdomleo7 merged 1 commit intojackdomleo7:masterfrom
tannerdolby:patch-119
Jan 21, 2023
Merged

Fix licensing comment appearing thrice in main CSS file#164
jackdomleo7 merged 1 commit intojackdomleo7:masterfrom
tannerdolby:patch-119

Conversation

@tannerdolby
Copy link
Contributor

Description

Fixes #119

The comment appearing thrice was because /*! Checka11y.css v1.3.3 | MIT License | github.com/jackdomleo7/Checka11y.css */ appeared in a few separate sass files:

  • src/warnings/checka11y-warnings.scss
  • src/errors/checka11y-errors.scss
  • src/checka11y.scss

Then we we compile all the SCSS together, those comments appear in the compiled CSS file based on the order of each @use rule in checka11y.scss.

Flow:

  1. checka11y.scss is the main SCSS file that loads the /warnings/* and /errors/* files.
@use "./warnings/checka11y-warnings.scss";
@use "./errors/checka11y-errors.scss";
  1. Since the warnings are loaded first, we can now look at the @use order in checka11y-warnings.scss.
@use "./variables" as *;
@use "./customisation" as *;
@use "../shared/variables" as *;
@use "../shared/customisation" as *;
@use "../shared/helpers" as *;
@use "../shared/messages/" as *;
@use "./features/" as *;
  1. The _customisation.scss partial gets loaded first (after _variables) so that partial is what appears first when compiling checka11y.css. There isn't anything special about _customisation.scss, its just first in the @use "loading" order when compiling the main css file, if we put the comment instead into _variables, then we would have to make sure its not loaded in many files otherwise the comment would appear multiple times like the original state.
  2. tldr; add the comment into the partial that gets loaded first into the main file, then it can appear once at the top of checka11y.css as expected.
/*! Checka11y.css v2.3.0 | MIT License | github.com/jackdomleo7/Checka11y.css */
:root {
  --checka11y-text-warning: #856404;
  --checka11y-bg-warning: #ffffd8;
  --checka11y-border-warning: #ff6;
}

:root {
  --checka11y-space: 0.25rem;
...
  1. I only added it to /warnings/* because if we add it to customisation.scss in /errors/* then we will see two comments in the compiled file like this:
/*! Checka11y.css v2.3.0 | MIT License | github.com/jackdomleo7/Checka11y.css */
:root {
  --checka11y-text-warning: #856404;
  --checka11y-bg-warning: #ffffd8;
  --checka11y-border-warning: #ff6;
}

/*! Checka11y.css v2.3.0 | MIT License | github.com/jackdomleo7/Checka11y.css */
:root {
  --checka11y-space: 0.25rem;
  --checka11y-space-0: calc(var(--checka11y-space) * 0);
...

Why is this important?

Fixes an annoying bug that was encountered awhile ago while working on another ticket.

Covered test cases

No test cases modified. Tests still pass as expected with npm run test.

Did you test on all major browsers?

  • Chrome
  • Firefox
  • Edge
  • Safari

Other details


T&Cs

  • I confirm I have read and understand the contributing guidelines
  • I understand the work in this pull request will not be released straight away and will appear in a future release (if approved)
  • I confirm the work in this pull request is true and valid to the best of my knowledge
  • I have updated the README, features.md and codes.md files where applicable

Copy link
Contributor

@alvaromontoro alvaromontoro left a comment

Choose a reason for hiding this comment

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

Looks good
🚢

Copy link
Owner

@jackdomleo7 jackdomleo7 left a comment

Choose a reason for hiding this comment

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

Haha this is annoying but good spot!

@jackdomleo7 jackdomleo7 added the bug Something isn't working quite right label Jan 21, 2023
@jackdomleo7 jackdomleo7 merged commit 4ca27b2 into jackdomleo7:master Jan 21, 2023
@jackdomleo7
Copy link
Owner

@tannerdolby so I didn't think of this when looking at your PR, but I created a shared/_license.scss file that gets imported vua @use into checka11y.scss, errors/checka11y-errors.scss and warnings/checka11y-warnings.scss.
I saw your comment regarding only using this comment in the warnings file because otherwise it'd be duplicated... well, it turns out that a nice feature of @use is that it won't duplicate, so even though I have written @use './shared/_license.scss 3 times, it only gets imported once, causing it to render as we expect for all 3 stylesheets.
No doubt though your PR got us closer to what we wanted, I was struggling to figure out what you found it.

See commit: ab3856f

@tannerdolby
Copy link
Contributor Author

tannerdolby commented Jan 21, 2023

@jackdomleo7 I'm glad I was able to help! Nice one using the _license.scss partial instead of the "hackish" solution I proposed. I just had a look at ab3856f and that is a much better approach. Stoked we were able to track this down. Feel free to ping me about any other issues in this project if you need an extra hand in the future.

Also, thanks for your review @alvaromontoro :)

@tannerdolby tannerdolby deleted the patch-119 branch January 21, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working quite right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Important comment is repeated thrice in main checka11y.css file

3 participants