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.

Sign up for GitHub

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

Jump to bottom

refactor: box shadow color #143

Merged
merged 4 commits into from
Dec 10, 2018
Merged

refactor: box shadow color #143

merged 4 commits into from
Dec 10, 2018

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Dec 9, 2018 •
edited
Loading

ref #60

Redo box-shadows on pseudo elements to use default color to reduce size. Added functionality to detect the most used color in icons based off this sass function count-values

  • balloons
  • containers
  • icons

Size Before:

npm run build time: 16.190s

file size
nes.css 699K
nes.css.map 55K
nes.min.css 668K

Size After:

npm run build time: 14.910s

file size
nes.css 560K
nes.css.map 57K
nes.min.css 531K

@jjspace jjspace changed the title refactor: box shadow color [WIP] refactor: box shadow color Dec 9, 2018
@trezy trezy added enhancement New feature or request and removed improvement labels Dec 9, 2018
@jjspace jjspace changed the title [WIP] refactor: box shadow color refactor: box shadow color Dec 10, 2018
BcRikko
BcRikko reviewed Dec 10, 2018
View reviewed changes
Copy link
Member

@BcRikko BcRikko left a comment

Choose a reason for hiding this comment

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

Great 🎉

Could you use the default color here? 🤔

NES.css/scss/elements/balloons.scss

Lines 10 to 22 in 73b3616

box-shadow:
0 -4px $background-color,
0 -8px $base-color,
4px 0 $background-color,
4px -4px $base-color,
8px 0 $base-color,
0 4px $background-color,
0 8px $base-color,
-4px 0 $background-color,
-4px 4px $base-color,
-8px 0 $base-color,
-4px -4px $base-color,
4px 4px $base-color;

@jjspace
Copy link
Contributor Author

jjspace commented Dec 10, 2018 •
edited
Loading

You could but I wouldn't. Because the default color is the color property it affects text too. ie. if you ever wanted a balloon with a different color and changed that color the text would also change like the pic below. This is why I only switched pseudo elements :after and :before because they don't have any text that would change.

Imgur

trezy
trezy approved these changes Dec 10, 2018
View reviewed changes
Copy link
Member

@trezy trezy left a comment

Choose a reason for hiding this comment

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

LGTM! 🎊

scss/utilities/icon-mixin.scss
$ret: "";
$moz: "";
@if ($default-color == null) {
Copy link
Member

Choose a reason for hiding this comment

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

This is really clever. Well done, @jjspace! 😉

@trezy trezy merged commit e0eb776 into nostalgic-css:develop Dec 10, 2018
@trezy trezy mentioned this pull request Dec 10, 2018
Closed
@BcRikko
Copy link
Member

BcRikko commented Dec 18, 2018

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

This was referenced Dec 18, 2018
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BcRikko BcRikko BcRikko left review comments

@trezy trezy trezy approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jjspace @BcRikko @trezy