Move theming support from Beta to Main #129
Conversation
|
|
Reviewer's Guide by SourceryThis pull request updates the auro-icon component to support theming and introduces several new features and changes. The main changes include:
The implementation focuses on enhancing the component's flexibility and aligning it with the latest design system guidelines. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jordanjones243 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding performance benchmarks or optimization techniques for rendering multiple icons, especially with the new custom SVG support.
- The deprecation of the
primaryattribute should be clearly communicated in the migration guide or release notes to help users update their implementations.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
test/auro-icon.test.js
Outdated
| expect(div).to.have.class('primary'); | ||
| expect(div).to.have.class('emphasis'); | ||
| expect(div).to.not.have.class('accent'); | ||
| expect(div).to.not.have.class('disabled'); | ||
| expect(nestedDiv).to.have.class('util_displayHiddenVisually'); |
There was a problem hiding this comment.
suggestion (testing): Test case needs to be updated to reflect new component behavior
The test case title and assertions no longer match the component's behavior. The emphasis style and primary class checks have been removed. Consider updating the test to focus on the a11y div being hidden by default, which seems to be the main assertion left.
| expect(nestedDiv).to.have.class('util_displayHiddenVisually'); | |
| it('auro-icon a11y div is hidden by default', async () => { | |
| const el = await fixture(html` | |
| <auro-icon category="interface" name="chevron-up"></auro-icon> | |
| `); | |
| await waitUntil(() => el.svg, 'Element did not become ready'); | |
| const nestedDiv = el.shadowRoot.querySelector('div > div'); | |
| expect(nestedDiv).to.have.class('util_displayHiddenVisually'); | |
| }); |
test/auro-icon.test.js
Outdated
| @@ -88,20 +90,15 @@ describe('auro-icon', () => { | |||
| expect(fetch).to.be.calledOnceWith(sinon.match('icons/interface/chevron-up.svg')); | |||
| }); | |||
There was a problem hiding this comment.
suggestion (testing): Remove outdated test case for accent style
This test case is checking for styles that are no longer part of the component's behavior. Consider removing this test or updating it to check for the new styling approach.
test/auro-icon.test.js
Outdated
| await expect(el.localName).to.equal('custom-icon'); | ||
|
|
||
| expect(el).to.have.attribute('auro-icon'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add tests for new attributes and behaviors
Consider adding test cases for the newly introduced attributes such as 'customSvg', 'info', 'secondary', 'tertiary', and 'subtle'. Also, add tests to verify the behavior of the new slot for custom SVG content.
it('renders custom SVG content', async () => {
const el = await fixture(html`
<auro-icon>
<svg slot="custom" viewBox="0 0 24 24"><path d="M12 2L2 22h20L12 2z"/></svg>
</auro-icon>
`);
expect(el.shadowRoot.querySelector('slot[name="custom"]')).to.exist;
});
it('applies secondary style', async () => {
const el = await fixture(html`<auro-icon category="interface" name="chevron-up" secondary></auro-icon>`);
expect(el).to.have.attribute('secondary');
});
src/auro-icon.js
Outdated
| title="${ifDefined(this.title ? this.title : undefined)}" | ||
| > | ||
|
|
||
| title="${ifDefined(this.title ? this.title : undefined)}"> |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
| title="${ifDefined(this.title ? this.title : undefined)}"> | |
| title="${ifDefined(this.title || undefined)}"> |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
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: #112
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.
Type 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
Move theming support from Beta to Main by introducing custom SVG support, adding new icon styles, and updating documentation and tests accordingly.
New Features:
customSvgattribute, allowing users to render any SVG content within theauro-iconcomponent.Enhancements:
info,secondary,subtle, andtertiaryto provide more styling options for icons.primaryicon style in favor of more specific style options.Documentation:
customSvgattribute and additional icon styles.primaryicon style.Tests:
auro-iconas a custom element with a custom name.advisorystyle and update tests to reflect new icon styles.