-
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
fix: use label as placeholder when there is no placeholder string set #36 #157
base: beta
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR changes the default placeholder for the auro-select component to an empty string. If a label is provided via the label slot, the label text will be used as the placeholder. If no placeholder is set and no label is provided, the placeholder will be empty. Sequence diagram for updated placeholder behavior in auro-selectsequenceDiagram
participant User
participant AuroSelect
participant Label
participant Placeholder
Note over AuroSelect: Initialize with empty placeholder
User->>AuroSelect: Provide label via slot
AuroSelect->>Label: handleLabelSlotChange()
Label-->>AuroSelect: Return label text
AuroSelect->>Placeholder: Use label as placeholder
Note over Placeholder: Display label text as placeholder
alt No label provided
User->>AuroSelect: No label slot content
AuroSelect->>Placeholder: Use empty placeholder
Note over Placeholder: Display empty placeholder
end
alt Custom placeholder set
User->>AuroSelect: Set placeholder property
AuroSelect->>Placeholder: Use custom placeholder
Note over Placeholder: Display custom placeholder text
end
State diagram for auro-select placeholder statesstateDiagram-v2
[*] --> EmptyPlaceholder: Initialize
EmptyPlaceholder --> LabelAsPlaceholder: Label provided
EmptyPlaceholder --> CustomPlaceholder: Placeholder set
LabelAsPlaceholder --> CustomPlaceholder: Placeholder set
CustomPlaceholder --> LabelAsPlaceholder: Placeholder cleared
LabelAsPlaceholder --> EmptyPlaceholder: Label removed
CustomPlaceholder --> EmptyPlaceholder: Placeholder cleared
state EmptyPlaceholder {
[*] --> ShowEmpty
}
state LabelAsPlaceholder {
[*] --> ShowLabelText
}
state CustomPlaceholder {
[*] --> ShowCustomText
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @sun-mota - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests for the new handleLabelSlotChange functionality to ensure the label-as-placeholder behavior works correctly
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: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
</span> | ||
<div class="menuWrapper"> | ||
</div> | ||
<slot name="label" slot="label"></slot> | ||
<span slot="label" class="${!this.placeholder && !this.value ? 'hidden' : ''}"> |
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.
I am a little confused here. It looks like you are hiding the label when there is no placeholder and there is no value.
Shouldn't, like auro-input, we always show the elements label regardless of placehodler/value conditions?
@@ -545,11 +566,13 @@ export class AuroSelect extends LitElement { | |||
chevron | |||
part="dropdown"> | |||
<span slot="trigger" aria-haspopup="true" id="triggerFocus"> | |||
${this.value ? this.displayValue : html`<span class="placeholder">${this.placeholder}</span>`} | |||
${this.value ? this.displayValue : html`<span class="placeholder">${this.placeholder || this.labelText}</span>`} |
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.
I think this is problematic for two reasons.
You have a span that you have identified as "placeholder" but you are also conditionally putting non-placeholder content into it. That's an anti-pattern we should avoid.
Also, the label for form elements shouldn't change as it will notably impact the experience a voice over utility relies on.
I think you are safer having two different elements that you simply hide/show the one you need based on state.
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.
Also, I think we should be keeping slot content out of properties.
Alaska Airlines Pull Request
Closes #36
Before Submitting this pull request:
Development
sectionnote: all pull requests require at least one linked ticket
Ready For Review
, all ticket's linked underDevelopment
must have their status changed toReady For Review
as 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
Bug Fixes: