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

Fixed #219 Checkbox icons appear the wrong color when checkbox import... #235

Merged
merged 5 commits into from
Aug 13, 2019
Merged

Fixed #219 Checkbox icons appear the wrong color when checkbox import... #235

merged 5 commits into from
Aug 13, 2019

Conversation

jianliao
Copy link
Contributor

@jianliao jianliao commented Aug 8, 2019

…ed before icons

Description

Checkbox icons appear the wrong color when checkbox imported before icons

The pen that demo the fix: https://codepen.io/jianliao/pen/gNNZOw

Related Issue

#219

Motivation and Context

@lazd 's original pen: https://codepen.io/lazd/pen/pXYGqd?editors=0010

How Has This Been Tested?

Latest Chrome + MacOS
Latest Firefox + MacOS
Latest Safari + MacOS

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@jianliao jianliao requested a review from lazd August 13, 2019 06:03
@jianliao jianliao self-assigned this Aug 13, 2019
@jianliao
Copy link
Contributor Author

jianliao commented Aug 13, 2019 •
edited

@lazd Finally got it working. So here is what's happening:

  1. "optionalDependences" in lerna managed packages will be moved to "dependencies" while hoisting. See if optional dependency installation fails, lerna bootstrap fails lerna/lerna#1879
  2. Must use npm install, because npm ci will still try to install optional dependency and fail in this case.
  3. Even changing the bootstrap script to npx lerna bootstrap --hoist -- --no-optional would not work

So the solution is that whenever we want to add a optional dependency, add it to top-level package.json.

Please review it and merge to lerna branch, so travis build for lerna branch becomes green. https://travis-ci.org/adobe/spectrum-css/builds/571167799

@lazd
Copy link
Member

lazd commented Aug 13, 2019

@jianliao looks great! Can you file an issue against lerna about the optionalDependencies being hoisted as dependencies?

@lazd lazd merged commit b5879eb into lerna Aug 13, 2019
@jianliao
Copy link
Contributor Author

Yes, I will. I think I need to do more investigation. It seems to me that lerna intends to do it this way.

lazd added a commit that referenced this pull request Aug 23, 2019
@lazd
Move to individually versioned packages with Lerna (#254)
06688fa 
* New, responsive site based on individually versioned components with silly fast navigation, lazy loaded deps as docs are viewed
  * Basic accessible search with build-time index generation
* Update README for individual versioning, add legacy README with legacy usage
  * Add documentation for site updating and architecture
  * Add docs for documentation generation, link to docs, remove references to Precursors
  * Add instructions for adding/editing components
  * Add docs to show how to update DNA and icons
  * Add docs for getting DNA variables from @spectrum-css/vars
  * Document issues with npm ci and installing optional dependencies

* Update to DNA 5.18.1
* Update icons to 4.1.0
* Audit DNA status for all components
* Add nvmrc
* Export a experimental tree-shakable DNA variable object from @spectrum-css/vars

* Update Textarea line-height and min-height, closes #231
* Fix spinner buttons appearing on Steppers in Firefox, closes #214
* Fix search field on iOS, closes #229 
* Fix Menu chevron vertical alignment, fixes #240
* Fix horizontal alignment of labels in anchor buttons, fixes #239
* Remove button outline when focused in Firefox, fixes #161
* Fix bar loader label in IE 11, fixes #242
* Fix Radio label margins when labels are below, fixes #246
* Fix Checkbox icon color when checkbox imported before icons, fixes #219 (#235)
* Add Adobe Clean UX support, closes #189  (#248)
* Fix fallback fonts for ar, he, ja, zh-Hans, zh-Hant, and ko, closes #232 (#248)
* Removed float from tags, fixes #218 (#237)
* Make Slider grab handle look right in docs, closes #255
* Add missing Asset docs, closes #256
* Add Menu .is-highlighted so we can indicate highlight without mis-using .is-open, closes #258 
* Support using links as menu items, closes #257
* Fix incorrect height for small BarLoader, fixes #259
@lazd lazd mentioned this pull request Aug 27, 2019
Merged
1 task
jianliao pushed a commit that referenced this pull request Aug 27, 2019
@lazd
Move to individually versioned packages with Lerna (#254)
567c5ed 
* New, responsive site based on individually versioned components with silly fast navigation, lazy loaded deps as docs are viewed
  * Basic accessible search with build-time index generation
* Update README for individual versioning, add legacy README with legacy usage
  * Add documentation for site updating and architecture
  * Add docs for documentation generation, link to docs, remove references to Precursors
  * Add instructions for adding/editing components
  * Add docs to show how to update DNA and icons
  * Add docs for getting DNA variables from @spectrum-css/vars
  * Document issues with npm ci and installing optional dependencies

* Update to DNA 5.18.1
* Update icons to 4.1.0
* Audit DNA status for all components
* Add nvmrc
* Export a experimental tree-shakable DNA variable object from @spectrum-css/vars

* Update Textarea line-height and min-height, closes #231
* Fix spinner buttons appearing on Steppers in Firefox, closes #214
* Fix search field on iOS, closes #229 
* Fix Menu chevron vertical alignment, fixes #240
* Fix horizontal alignment of labels in anchor buttons, fixes #239
* Remove button outline when focused in Firefox, fixes #161
* Fix bar loader label in IE 11, fixes #242
* Fix Radio label margins when labels are below, fixes #246
* Fix Checkbox icon color when checkbox imported before icons, fixes #219 (#235)
* Add Adobe Clean UX support, closes #189  (#248)
* Fix fallback fonts for ar, he, ja, zh-Hans, zh-Hant, and ko, closes #232 (#248)
* Removed float from tags, fixes #218 (#237)
* Make Slider grab handle look right in docs, closes #255
* Add missing Asset docs, closes #256
* Add Menu .is-highlighted so we can indicate highlight without mis-using .is-open, closes #258 
* Support using links as menu items, closes #257
* Fix incorrect height for small BarLoader, fixes #259
jianliao pushed a commit that referenced this pull request Sep 19, 2019
@lazd @jianliao
feat: move to individually versioned packages with Lerna (#265)
cb7fd4b 
Update Textarea line-height and min-height, closes #231
Fix spinner buttons appearing on Steppers in Firefox, closes #214
Fix search field on iOS, closes #229
Fix Menu chevron vertical alignment, fixes #240
Fix horizontal alignment of labels in anchor buttons, fixes #239
Remove button outline when focused in Firefox, fixes #161
Fix bar loader label in IE 11, fixes #242
Fix Radio label margins when labels are below, fixes #246
Fix Checkbox icon color when checkbox imported before icons, fixes #219 (#235)
Add Adobe Clean UX support, closes #189 (#248)
Fix fallback fonts for ar, he, ja, zh-Hans, zh-Hant, and ko, closes #232 (#248)
Removed float from tags, fixes #218 (#237)
Make Slider grab handle look right in docs, closes #255
Add missing Asset docs, closes #256
Add Menu .is-highlighted so we can indicate highlight without mis-using .is-open, closes #258
Support using links as menu items, closes #257
Fix incorrect height for small BarLoader, fixes #259
@GarthDB GarthDB deleted the issue-219-checkbox-icon-lerna branch September 21, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lazd lazd Awaiting requested review from lazd

Assignees

@jianliao jianliao

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jianliao @lazd