-
Notifications
You must be signed in to change notification settings - Fork 0
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
Address color theming issues and tech debt #64
Conversation
Reviewer's Guide by SourceryThis pull request addresses color theming issues and technical debt in the auro-banner component. The changes include updating color variables, improving component versioning, refactoring the component registration process, and enhancing the banner's functionality with dynamic header tags. 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 - here's some feedback:
Overall Comments:
- The color theming improvements and use of CSS custom properties are good changes. However, the new component registration process using RuntimeUtils seems to add unnecessary complexity. Could you explain the benefits of this change or consider reverting to the simpler registerComponent approach?
- The addition of headerVersion.js file for versioning is concerning. This approach might lead to version mismatches if not updated properly. Consider alternative ways to handle versioning that are less prone to errors.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -43,7 +43,7 @@ describe('auro-banner', () => { | |||
<auro-banner hero></auro-banner> | |||
`); | |||
|
|||
const header = el.shadowRoot.querySelector('auro-header'); |
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): Update test selector to match new dynamic header tag implementation
The test has been updated to use an attribute selector instead of a tag name selector. This change aligns with the new dynamic header tag implementation in the component. However, we should consider adding more specific tests to ensure the correct header tag is being used based on the version.
const header = el.shadowRoot.querySelector('[slot="header"]');
const headerTag = header.tagName.toLowerCase();
expect(['h1', 'h2', 'h3', 'h4', 'h5', 'h6']).to.include(headerTag);
@@ -43,7 +43,7 @@ describe('auro-banner', () => { | |||
<auro-banner hero></auro-banner> | |||
`); | |||
|
|||
const header = el.shadowRoot.querySelector('auro-header'); | |||
const header = el.shadowRoot.querySelector('[auro-header]'); | |||
|
|||
await expect(header).to.not.be.null; | |||
}); |
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 functionality
The PR introduces new functionality such as version-based header tag generation and runtime utilities. Consider adding tests to cover these new features, ensuring they work as expected across different scenarios.
});
it('should generate correct header tag based on version', async () => {
const el = await fixture(html`<auro-banner></auro-banner>`);
const header = el.shadowRoot.querySelector('[auro-header]');
expect(header.tagName.toLowerCase()).to.equal('h2');
el.setAttribute('version', '2');
await el.updateComplete;
expect(header.tagName.toLowerCase()).to.equal('h3');
});
demo/index.md
Outdated
import './node_modules/@aurodesignsystem/auro-banner'; | ||
registerComponent('custom-banner'); | ||
import { AuroBanner } from './src/auro-banner.js'; | ||
import * as RuntimeUtils from '@aurodesignsystem/auro-library/scripts/utils/runtimeUtils.mjs'; | ||
|
||
RuntimeUtils.default.prototype.registerComponent('custom-banner', AuroBanner); | ||
``` | ||
|
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): Update documentation for new import and registration pattern
The import and registration method for the component has changed significantly. Is there updated documentation available that explains this new pattern and its benefits?
src/auro-banner.js
Outdated
@@ -44,15 +49,29 @@ export class AuroBanner extends LitElement { | |||
this.iconic = false; | |||
this.marquee = false; | |||
this.roundedBorder = false; | |||
|
|||
const versioning = new AuroDependencyVersioning(); |
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.
issue (complexity): Consider moving versioning logic and runtime utilities out of the banner component.
The changes introduce unnecessary complexity by adding versioning and runtime utilities directly into the banner component. To simplify:
-
Move versioning logic to a higher-level wrapper or build step:
// In a separate utility file or build process const headerTag = generateVersionedTag('auro-header', headerVersion, AuroHeader); // In AuroBanner import { headerTag } from './versionedComponents'; // Use headerTag directly in render method render() { return html` <${headerTag} level="2" display="600" margin="both" size="none" class="title"> <slot name="title"></slot> </${headerTag}> `; }
-
Remove runtime utilities from the component:
// Remove these lines import AuroLibraryRuntimeUtils from '@aurodesignsystem/auro-library/scripts/utils/runtimeUtils.mjs'; this.runtimeUtils = new AuroLibraryRuntimeUtils(); // Handle renaming at a higher level if necessary
-
Simplify the constructor:
constructor() { super(); this.hero = false; this.iconic = false; this.marquee = false; this.roundedBorder = false; }
These changes will maintain functionality while reducing complexity, improving readability, and keeping the banner component focused on its primary purpose.
🎉 This PR is included in version 2.1.0-beta.2 🎉 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: #57
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
Address color theming issues by updating CSS variables to align with design system tokens. Introduce dynamic header tag generation and refactor component registration using RuntimeUtils for better version management. Update documentation and tests to reflect these changes, and remove deprecated Husky script.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: