feat: initial integration of floatingUI and dropdown POC #3
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Class diagram for the new auro-dropdown componentclassDiagram
class AuroDropdown {
+Boolean bordered
+Boolean chevron
+Boolean disabled
+Boolean error
+Boolean inset
+Boolean matchWidth
+Boolean rounded
+Boolean hoverToggle
+Boolean noToggle
+Boolean focusShow
+Boolean noHideOnThisFocusLoss
+Boolean isPopoverVisible
+Number dropdownWidth
+String placement
+Number tabIndex
+Function onSlotChange
+AuroLibraryRuntimeUtils runtimeUtils
+AuroFloatingUI floater
+Object floaterConfig
+String iconTag
+void privateDefaults()
+void connectedCallback()
+void disconnectedCallback()
+void updated(changedProperties)
+void firstUpdated()
+void handleDefaultSlot()
+html render()
}
class AuroFloatingUI {
+void position()
+void updateState()
+void setupHideHandlers()
+void handleUpdate(changedProperties)
+void updateCurrentExpandedDropdown()
+void showBib()
+void hideBib()
+void handleClick()
+void handleEvent(event)
+void handleTriggerTabIndex()
+void configure(elem)
}
AuroDropdown --> AuroFloatingUI
AuroDropdown --> AuroDropdownBib
AuroDropdown --> AuroLibraryRuntimeUtils
AuroDropdown --> AuroDependencyVersioning
AuroDropdown --> AuroIcon
AuroDropdown --> AuroDropdownBib
AuroDropdown --> AuroFloatingUI
AuroDropdown --> AuroLibraryRuntimeUtils
AuroDropdown --> AuroDependencyVersioning
AuroDropdown --> AuroIcon
class AuroDropdownBib {
+html render()
}
class MySelect {
+String value
+void configureBibContent()
+void firstUpdated()
+void updated(changedProperties)
+html render()
}
MySelect --> AuroDropdown
AuroDropdown --> AuroDropdownBib
AuroDropdown --> AuroFloatingUI
AuroDropdown --> AuroLibraryRuntimeUtils
AuroDropdown --> AuroDependencyVersioning
AuroDropdown --> AuroIcon
class AuroLibraryRuntimeUtils {
}
class AuroDependencyVersioning {
+String generateTag(componentName, version, component)
}
class AuroIcon {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jason-capsule42 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests for this new component to ensure reliability and ease future maintenance.
- There's a significant amount of commented-out code and TODO comments. Please clean these up before merging to improve code readability.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try { | ||
| this.element.cleanup(); | ||
| // this.element.bib.remove(); // is this doing anything? Seems like it's doing the wrong thing | ||
| } catch (error) { |
There was a problem hiding this comment.
suggestion (bug_risk): Improve error handling in catch blocks
Several catch blocks in the code are empty or only contain a comment. Consider logging these errors or handling them appropriately to avoid masking important issues.
| } catch (error) { | |
| } catch (error) { | |
| console.warn('Error during element cleanup:', error); | |
| } |
| * @event auroDropdown-ready - Notifies that the component has finished initializing. | ||
| * @event auroDropdown-toggled - Notifies that the visibility of the dropdown bib has changed. | ||
| */ | ||
| export class AuroDropdown extends LitElement { |
There was a problem hiding this comment.
suggestion: Enhance accessibility features of the dropdown component
Ensure that the dropdown component is fully accessible. This includes making it keyboard navigable, ensuring it works well with screen readers, and following ARIA best practices for dropdown menus. Consider adding appropriate ARIA attributes and roles.
export class AuroDropdown extends LitElement {
static get properties() {
return {
expanded: { type: Boolean, reflect: true },
label: { type: String }
};
}
constructor() {
super();
this.expanded = false;
this.addEventListener('keydown', this._handleKeyDown);
}
| <!-- The below content is automatically added from ./../docs/partials/description.md --> | ||
| `<auro-form>` is a [HTML custom element](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements) for the purpose of ... | ||
|
|
||
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nullam convallis in tellus nec pellentesque. Integer bibendum ligula sit amet vehicula gravida. Maecenas accumsan, ligula vitae molestie iaculis, tellus mi laoreet ex [install instructions](https://auro.alaskaair.com/components/auro/button/install), ac malesuada velit dolor vel mi. Cras et rutrum urna. Sed mattis mi eu tortor ullamcorper, egestas bibendum mauris cursus. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus viverra eros eget neque commodo vulputate. In tempus eu velit at dictum. |
There was a problem hiding this comment.
issue (documentation): Typo: Repeated 'the' in the description
There's a small typo in the description. 'the the' is repeated in the sentence 'This file is generated based on a template fetched from the the docs are compiled.'
| import { AuroDropdown } from "./auro-dropdown.js"; | ||
|
|
||
|
|
||
| export class MySelect extends LitElement { |
There was a problem hiding this comment.
issue (complexity): Consider extending AuroDropdown instead of creating a new component.
The MySelect component introduces an unnecessary layer of abstraction by wrapping AuroDropdown. Consider extending AuroDropdown instead of creating a new component. This would simplify the code and reduce complexity while maintaining the desired functionality. Here's a suggested approach:
import { AuroDropdown } from "./auro-dropdown.js";
export class MySelect extends AuroDropdown {
static get properties() {
return {
...super.properties,
value: {
type: String,
reflect: true
}
};
}
firstUpdated(changedProperties) {
super.firstUpdated(changedProperties);
this.configureBibContent();
}
configureBibContent() {
try {
this.inputElement = this.bibContent.querySelector('#selectInput');
this.inputElement.addEventListener('input', (event) => {
this.value = event.target.value;
});
} catch (error) {
// Handle error
}
}
updated(changedProperties) {
super.updated(changedProperties);
if (changedProperties.has('value')) {
console.warn('select.value updated to', this.value);
}
}
}
if (!customElements.get("my-select")) {
customElements.define("my-select", MySelect);
}This approach extends AuroDropdown, eliminating the need for a separate wrapper component. It maintains the custom value property and the configureBibContent method while leveraging the existing functionality of AuroDropdown.
If there are specific reasons for the current implementation that require a separate component, please clarify these requirements so we can suggest a more tailored solution.
| if (!this.element.noHideOnThisFocusLoss && !this.element.hasAttribute('noHideOnThisFocusLoss')) { // is this the right place to do this? | ||
| if (document.activeElement !== document.querySelector('body') && !this.element.contains(document.activeElement) && !this.element.bibContent.contains(document.activeElement)) { | ||
| this.hideBib(); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if (!this.element.noHideOnThisFocusLoss && !this.element.hasAttribute('noHideOnThisFocusLoss')) { // is this the right place to do this? | |
| if (document.activeElement !== document.querySelector('body') && !this.element.contains(document.activeElement) && !this.element.bibContent.contains(document.activeElement)) { | |
| this.hideBib(); | |
| } | |
| } | |
| if (!this.element.noHideOnThisFocusLoss && !this.element.hasAttribute('noHideOnThisFocusLoss') && (document.activeElement !== document.querySelector('body') && !this.element.contains(document.activeElement) && !this.element.bibContent.contains(document.activeElement))) { | |
| this.hideBib(); | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
Alaska Airlines Pull Request
Review Considerations
This PR includes changes that alter the current:
By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I have performed a self-review of my own update.
Summary by Sourcery
Integrate Floating UI with a new dropdown component as a proof of concept, introducing 'auro-dropdown' and 'MySelect' components, along with supporting styles and documentation.
New Features:
Enhancements:
Documentation: