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

Set up color maps for Decanter and refactor main nav colors to refer to color maps #420

Merged
merged 32 commits into from
Jun 11, 2019
Merged

Set up color maps for Decanter and refactor main nav colors to refer to color maps #420

merged 32 commits into from
Jun 11, 2019

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented May 31, 2019 •
edited
Loading

READY FOR REVIEW

Summary

  • Set up color maps for Decanter - a main one $core-colors and one for main-nav $main-nav-colors. There is already an existing one called $alert-colors.
  • Add a function color to get colors from color maps.
  • Refactor main-nav and a few other components to pull colors from the color map
  • Also included issue 422 - changed all reference to $color-dark-red to $color-bright-red for accessibility

Needed By (Date)

  • Whenever

Urgency

  • Not urgent

Steps to Test

  1. Pull this branch
  2. See that the main nav, buttons, brand bar, logo, global footer, alerts, links and all variants still have the correct colors.
  3. Look at the new color maps in utilities/variables and the function color.
  4. See that all places in main-nav with $color-dark-red have been changed to $color-bright-red.

Affected Projects or Products

  • Decanter

Associated Issues and/or People

yvonnetangsu added 24 commits May 28, 2019 14:53
@yvonnetangsu
WIP color maps with function to retrieve color; refactor brand bar an…
406c15d 
…d alerts to use color map and function; start refactoring main nav colors
@yvonnetangsu
WIP refactor main nav colors
b7da694
@yvonnetangsu
WIP refactor main nav colors
2d08215
@yvonnetangsu
Add warnings to color function
39d28eb
@yvonnetangsu
finish up main nav default style color refactor
c2466b8
@yvonnetangsu
Finish up refactoring main-nav--dark colors
d8c7295
@yvonnetangsu
Fix main-nav--light variant, when mobile search is present the backgr…
8b25375 
…ound color is now white; beginning color map refactoring
@yvonnetangsu
WIP main-nav--light color refactor
9d3de5a
@yvonnetangsu
WIP main-nav--light refactoring; change link-border to border
4f9cd86
@yvonnetangsu
Finish refactoring main-nav--light colors to use color map
38f290e
@yvonnetangsu
Refactor to replace .su-main-nav.su-main-nav--dark selectors with jus…
7ea298e 
…t .su-main-nav--dark
@yvonnetangsu
fixup su-main-nav--dark
6f7bc0d
@yvonnetangsu
Continue cleanup of main nav styles
6b3c15f
@yvonnetangsu
Main-nav code cleanup remove redundancy
f079dcb
@yvonnetangsu
WIP fixup
eb04eb2
@yvonnetangsu
bg color transition fixup
8d28b55
@yvonnetangsu
main-nav--light mobile minus sign fixup
a07c704
@yvonnetangsu
have colors in brand bar, global footer, logo, button primary to refe…
597cea0 
…r to color map
@yvonnetangsu
main-nav--light fixup
aebbeb3
@yvonnetangsu
main-nav--light fixup
6df7525
@yvonnetangsu
doc, styleguide and button/link colors
a0531d4
@yvonnetangsu
fixup
eeb056e
@yvonnetangsu
Doc
0e01215
@yvonnetangsu
cc indentation fix
ee9e703
@yvonnetangsu yvonnetangsu requested review from sherakama and JBCSU June 5, 2019 20:14
@yvonnetangsu
Copy link
Member Author

yvonnetangsu commented Jun 10, 2019 •
edited
Loading

@sherakama @JBCSU I've removed the nesting in the color maps and changed the function @color to get rid of the $theme parameter. Also, I've changed all reference to $color-dark-red to $color-bright-red per @kerri-augenstein 's suggestion (#422). Could you please give it another look? Thanks!

@JBCSU JBCSU mentioned this pull request Jun 10, 2019
Merged
sherakama
sherakama reviewed Jun 10, 2019
View reviewed changes
core/src/scss/components/main-nav/_main-nav--dark.scss Outdated
background-color: $color-white;
}
.su-main-nav--dark {
$_color-map: $main-nav-dark-colors;
Copy link
Member

Choose a reason for hiding this comment

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

Just in case this variable name is used somewhere else, can you prefix it with the component name.

eg: $main-nav-color-map

JBCSU
JBCSU approved these changes Jun 10, 2019
View reviewed changes
Copy link
Collaborator

@JBCSU JBCSU left a comment

Choose a reason for hiding this comment

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

I agree with @sherakama's request to rename the $main-nav*-colors variables as $su=main-nav*-colors. Otherwise, I think we're GTG. I'll approve now so your not blocked from merging while I'm away.

@yvonnetangsu
Copy link
Member Author

@sherakama - this one ready for a final look also. I'm a bit puzzled about why the custom error message isn't showing up if I tried putting a non-existent color for the color function 🤔

sherakama
sherakama approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

GTG. Thanks for this.

@sherakama
Copy link
Member

Not sure why CC is complaining about the line length. I'm pretty sure we have that set to 120.

@yvonnetangsu
Copy link
Member Author

Not sure why CC is complaining about the line length. I'm pretty sure we have that set to 120.

Yeah, CC's been weird lately. It complains about the braces not on their own lines but they are :\

@yvonnetangsu yvonnetangsu merged commit 0f7a682 into master Jun 11, 2019
@yvonnetangsu yvonnetangsu deleted the 386-nav-color-map branch June 11, 2019 18:02
yvonnetangsu added a commit that referenced this pull request Jun 11, 2019
@yvonnetangsu
Merge branch 'master' into 222-masthead
2d83f76 
* master: (44 commits)
  Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)
  Move PR template (#416)
  Fix skiplink target so it doesn't take up space and cause overflow (#415)
  Release 5.0.1 (#417)
  Fix CSS grid gaps not displaying on Edge issue and other minor grid related issues (#413)
  More link fixup (#388)
  forgot to rebuild .js after final lint fixes (#387)
  add openNav, closeNav, openSubnav, closeSubnav events (#379)
  Fix main nav color variants and work with new linting rules (#380)
  add favicon to style guide (#377)
  Adjust linters. (#370)
  update readme.md to match Scott's intro on homepage.md (#371)
  Update README.md (#369)
  move the images we need for the styleguide's homepage into kss-assets (#368)
  Update README.md (#367)
  Update UPGRADE.md (#363)
  package.locl.json file is back (#366)
  Added more to changelog (#364)
  Update homepage.md (#362)
  90 basic webpack (#334)
  ...

# Conflicts:
#	core/scss/components/index.scss
JBCSU added a commit that referenced this pull request Jun 11, 2019
@JBCSU
Fix a few CC complains.
7be0926 
Merge branch 'master' into 424-inline-link-style

* master:
  Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)

# Conflicts: - all resolved using "mine"
#	core/dist/css/decanter.css
#	core/src/scss/components/global-footer/_global-footer.scss
#	core/src/scss/utilities/functions/_color.scss
#	core/src/scss/utilities/mixins/button/_button-big.scss
#	core/src/scss/utilities/mixins/button/_button-primary.scss
#	core/src/scss/utilities/mixins/button/_button-secondary.scss
#	core/src/scss/utilities/mixins/link/_link.scss
#	core/src/scss/utilities/variables/core/_core-color-maps.scss
yvonnetangsu added a commit that referenced this pull request Jun 12, 2019
@yvonnetangsu
Merge branch 'master' into 423-svg-icon-test
328db3a 
* master:
  424 default link style (#427)
  Update package.json (#438)
  Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JBCSU JBCSU JBCSU approved these changes

@sherakama sherakama sherakama approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yvonnetangsu @sherakama @JBCSU