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

fix(icon): Fix icon color references #733

Merged
merged 2 commits into from
Jun 12, 2020
Merged

fix(icon): Fix icon color references #733

merged 2 commits into from
Jun 12, 2020

Conversation

anicholls
Copy link
Contributor

Summary

I missed a few things in #706. This addresses those issues.

@cypress
Copy link

cypress bot commented Jun 11, 2020 •
edited
Loading


Test summary

369 • 0 • 1 • 0

Run details
Project canvas-kit
Status Passed
Commit a112b91 ℹ️
Started Jun 12, 2020 6:56 PM
Ended Jun 12, 2020 7:01 PM
Duration 04:07 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lychyi
lychyi reviewed Jun 11, 2020
View reviewed changes
modules/icon/css/README.md
@@ -253,7 +253,7 @@ Default: `24`
## Default Options

```js
initializeIcons((iconPath = null), (selector = '.wdc-icon'), iconRoot = document);
initializeIcons((iconPath = null), (selector = '.wdc-icon'), (iconRoot = document));
Copy link
Contributor

Choose a reason for hiding this comment

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

😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linter did this, not me!

@anicholls anicholls added the ready for review Code is ready for review label Jun 11, 2020
@lychyi
Copy link
Contributor

lychyi commented Jun 12, 2020

@anicholls If this addresses #706, I'm not clear on why this PR is against master? Are some things broken today that we need to address before we merge master into prerelease/v4?

The resulting colors seem to be the same regardless of whether we use primary. or iconColors. right?

@anicholls
Copy link
Contributor Author

anicholls commented Jun 12, 2020 •
edited
Loading

@lychyi Oh wow. Major brain fart. Sorry, I will change the base to prerelease.

The resulting colors seem to be the same regardless of whether we use primary. or iconColors. right?

Yes, however primary no longer exists in 2.0

@anicholls anicholls changed the base branch from master to prerelease/v4 June 12, 2020 17:26
@anicholls anicholls merged commit a073caa into Workday:prerelease/v4 Jun 12, 2020
@anicholls anicholls deleted the fix-icon-colors branch June 12, 2020 19:38
This was referenced Jun 12, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lychyi lychyi lychyi approved these changes

Assignees
No one assigned
Labels
ready for review Code is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anicholls @lychyi