feat: initial integration of floatingUI and dropdown POC #4
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Sequence diagram for auro-dropdown interactionssequenceDiagram
actor User
participant AuroDropdown
participant AuroFloatingUI
participant AuroDropdownBib
User->>AuroDropdown: Click on trigger
AuroDropdown->>AuroFloatingUI: handleClick()
AuroFloatingUI->>AuroDropdown: showBib()
AuroDropdown->>AuroDropdownBib: Display dropdown content
AuroFloatingUI->>AuroDropdown: setupHideHandlers()
User->>AuroDropdown: Click outside
AuroFloatingUI->>AuroDropdown: hideBib()
AuroDropdown->>AuroDropdownBib: Hide dropdown content
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 noToggle
+Boolean noHideOnThisFocusLoss
+Boolean isPopoverVisible
+Boolean ready
+Number dropdownWidth
+String placement
+Number tabIndex
+void privateDefaults()
+void connectedCallback()
+void disconnectedCallback()
+void updated(changedProperties)
+void firstUpdated()
+void handleDefaultSlot()
+render()
}
class AuroDropdownBib {
+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 --> AuroDropdownBib
AuroDropdown --> AuroFloatingUI
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 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.
| // this.dispatchEvent(event); | ||
| // } | ||
|
|
||
| // applyBibContentFunctionality(bibElem) { |
There was a problem hiding this comment.
suggestion: Remove or uncomment and use commented-out code
There are several blocks of commented-out code in this file. If this code is no longer needed, it should be removed. If it's still needed, it should be uncommented and used. Keeping commented-out code can lead to confusion and maintenance issues.
| // applyBibContentFunctionality(bibElem) { | |
| applyBibContentFunctionality(bibElem) { | |
| if (bibElem) { | |
| // Implementation goes here | |
| } | |
| } |
| 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 code snippet is automatically added from ./../apiExamples/basic.html --> | ||
|
|
||
| ```html | ||
| <auro-form>Hello World</auro-form> |
There was a problem hiding this comment.
question (documentation): Consider additional API examples
Are there other use cases or configurations of the component that could be showcased here? Additional examples could help users understand the full capabilities of the component.
| 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
Before Submitting this pull request:
Developmentsectionnote: all pull requests require at least one linked ticket
Ready For Review, all ticket's linked underDevelopmentmust have their status changed toReady For Reviewas wellBy 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 FloatingUI and introduce a new dropdown component as a proof of concept. The 'auro-dropdown' component is designed to be interactive and customizable, supporting various features like chevron display and event-driven visibility toggling. Additionally, a 'MySelect' component is added to demonstrate the dropdown's usage in a selectable input context. Documentation is provided to guide users on installation and usage.
New Features:
Enhancements:
Documentation: