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
Use handleName field consistently in HandleController #2036
Conversation
| @@ -208,7 +208,7 @@ export class HandleController implements Controller { | |||
| handles.forEach((handle, index) => { | |||
| /* c8 ignore next */ | |||
| if (!handle.handleName?.length) { | |||
| handle.name = `handle${index + 1}`; | |||
| handle.handleName = `handle${index + 1}`; | |||
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.
This seems weird in that handle.handleName is only a getter for handle.name...
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.
spectrum-web-components/packages/slider/src/SliderHandle.ts
Lines 80 to 82 in d23f735
| public get handleName(): string { | |
| return this.name; | |
| } |
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.
Yeah, I can't explain it. My template looks like this, i.e. there's definitely a name there:
<sp-slider variant="range" step="1">
<sp-slider-handle slot="handle" name="min" label="Minimum" max="next"></sp-slider-handle>
<sp-slider-handle slot="handle" name="max" label="Maximum" min="previous"></sp-slider-handle>
</sp-slider>(The slider's min/max attributes and the handles' value attributes are filled in when processing the template.)
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.
Should've said, my "fix" is wrong (in editing the PR description while I was waiting for CLA approval I removed that minor detail) but I'm not sure why the handle's name property isn't being initialized properly.
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.
Could you share into the issue more info around how you add the DOM to the page, etc.?
As this change isn't right, let's close this PR and discuss finding the source of this in the issue.
|
Let's look more for a source before we review a fix. |
Description
There are two instances of
handle.namethat should behandle.handleNameso that the handle map is populated correctly.Related issue(s)
Motivation and context
I haven't nailed down the exact conditions, but sometimes when adding
sp-sliders dynamically from an HTML template only one instance ofsp-slider-handlewill be added because the handle map key is undefined. (I finally tracked down the behaviour when I started gettingFirst slider handle cannot have attribute min="previous"after addingmax="next"to the firstsp-slider-handleandmin="previous"to the second.)How has this been tested?
I'm afraid I don't have a reduced test case, but I was able to track down the behaviour in my (currently non-public) app. The code does something like this (which doesn't reproduce the issue in isolation, but is fixed by the change in this PR):
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.