-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Implement color theming support #199
Conversation
Reviewer's Guide by SourceryThis pull request implements color theming support and introduces a custom versioned Key implementation details:
These changes significantly improve the component's customizability and maintainability while introducing theme support. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jordanjones243 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
2bd2757
to
04d6915
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
77de448
to
0955810
Compare
0955810
to
6b686bb
Compare
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jordanjones243 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The removal of the CSS custom property fallbacks section from the README might impact users supporting older browsers. Was this intentional, and if so, how should users handle fallbacks now?
- Some specific color styles have been removed from style.scss. Please confirm that these are now properly handled by the new color theming system and that there are no unintended side effects from this change.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -112,7 +112,7 @@ describe('auro-select', () => { | |||
|
|||
await elementUpdated(el); | |||
|
|||
const dropdown = el.shadowRoot.querySelector('auro-dropdown'); | |||
const dropdown = el.shadowRoot.querySelector('[auro-dropdown]'); | |||
const triggerContentHTML = dropdown.querySelector('#triggerFocus').innerHTML; | |||
|
|||
await expect(el.optionSelected).to.be.equal(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add tests for new color theming support
The pull request implements color theming support, but there are no tests to verify this new functionality. Consider adding tests that check if the component correctly applies theme-related CSS variables and classes.
await expect(el.optionSelected).to.be.equal(undefined); | |
await expect(el.optionSelected).to.be.equal(undefined); | |
// Test color theming support | |
el.setAttribute('theme', 'dark'); | |
await el.updateComplete; | |
expect(el.classList.contains('auro-select--dark')).to.be.true; | |
el.setAttribute('theme', 'light'); | |
await el.updateComplete; | |
expect(el.classList.contains('auro-select--light')).to.be.true; |
### CSS Custom Property fallbacks | ||
|
||
<!-- AURO-GENERATED-CONTENT:START (REMOTE:url=https://raw.githubusercontent.com/AlaskaAirlines/WC-Generator/master/componentDocs/partials/usage/cssFallbacks.md) --> | ||
[CSS custom properties](https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties) are [not supported](https://auro.alaskaair.com/support/custom-properties) in older browsers. For this, fallback properties are pre-generated and included with the npm. | ||
|
||
Any update to the Auro Design Tokens will be immediately reflected with browsers that support CSS custom properties, legacy browsers will require updated components with pre-generated fallback properties. | ||
|
||
<!-- AURO-GENERATED-CONTENT:END --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (documentation): Removal of CSS Custom Property fallbacks section
Could you provide the rationale for removing this section? It contained information about browser support for CSS custom properties, which might still be relevant for users supporting older browsers.
PR was approved with @jason-capsule42 during pair programming and I will therefore be forcing a release. |
🎉 This PR is included in version 2.11.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Alaska Airlines Pull Request
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Resolves: #198
Summary:
Please summarize the scope of the changes you have submitted, what the intent of the work is and anything that describes the before/after state of the project.
auro-dropdown
componentType of change:
Please delete options that are not relevant.
Checklist:
By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.
Thank you for your submission!
-- Auro Design System Team
Summary by Sourcery
Implement color theming support by adding tier 3 color tokens and refactor the
auro-dropdown
component to use a custom versioned tag. Update documentation and tests to reflect these changes, and remove deprecated husky script.New Features:
Enhancements:
auro-dropdown
component to use a custom versioned tag for better dependency management.Documentation:
Tests:
auro-dropdown
component's query selector.Chores: