1042-form-field-mixin #5809
1042-form-field-mixin #5809
Conversation
feat(label): init field label mixin
…5732) * fix(menu): added check to find focused element within root context * fix(menu): added story * fix(menu): added test * chore(menu): added changeset * fix(menu): added global const for component input pattern * fix(menu): remove delimiter from the regexp constructor * chore: skipped prod and vrt tests on the new story * chore: fix tests helpers * fix: check for cross root boundary * fix: code comments --------- Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]>
🦋 Changeset detectedLatest commit: 287fe7b The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "component", | ||
| "css" | ||
| ], | ||
| "dependencies": { |
There was a problem hiding this comment.
Expected object keys to be in specified order. 'dependencies' should be before 'keywords'.
reviewdog suggestion error
GitHub comment range and suggestion line range must be same. L66-L66 v.s. L53-L80| "@spectrum-web-components/shared": "1.9.0", | ||
| "@spectrum-web-components/textfield": "1.9.0" | ||
| }, | ||
| "types": "./src/index.d.ts", |
There was a problem hiding this comment.
Expected object keys to be in specified order. 'types' should be before 'dependencies'.
reviewdog suggestion error
GitHub comment range and suggestion line range must be same. L81-L81 v.s. L53-L81| "development": "./src/index.dev.js", | ||
| "default": "./src/index.js" | ||
| }, | ||
| "./sp-field-label-mixin.js": { |
There was a problem hiding this comment.
Expected object keys to be in ascending order. './sp-field-label-mixin.js' should be before './src/index.js'.
reviewdog suggestion error
GitHub comment range and suggestion line range must be same. L42-L42 v.s. L27-L44| superClass: T, | ||
| slotName?: string, | ||
| excludedSelectors: string[] = [] | ||
| ) => { |
There was a problem hiding this comment.
Missing return type on function.
| "component", | ||
| "css" | ||
| ], | ||
| "dependencies": { |
There was a problem hiding this comment.
Expected object keys to be in specified order. 'dependencies' should be before 'keywords'.
reviewdog suggestion error
GitHub comment range and suggestion line range must be same. L66-L66 v.s. L53-L74
Tachometer resultsChromemenu permalinktest-basic
Firefoxmenu permalinktest-basic
|
|
|
||
| ```html | ||
| <sp-color-field size="s" value="#ffff00"></sp-color-field> | ||
| <sp-color-field |
There was a problem hiding this comment.
Label can be an attribute.
|
|
||
| ```html | ||
| <sp-color-field view-color value="#f00"></sp-color-field> | ||
| <sp-color-field view-color value="#f00">Icon color</sp-color-field> |
There was a problem hiding this comment.
Or label can be slotted.
| * @slot tooltip - Tooltip to to be applied to the the Picker Button | ||
| */ | ||
| export class Combobox extends Textfield { | ||
| export class Combobox extends FieldLabelMixin(Textfield, 'field-label') { |
There was a problem hiding this comment.
Since combobox uses the default slot for menuitems, we needed a field-label slot for the visual label
| `; | ||
| } | ||
|
|
||
| protected override get _ariaLabel(): string | undefined { |
There was a problem hiding this comment.
we need to override textfieldBase's logic to include the labelling that exists in combobox
| id="combobox-disabled-items" | ||
| style="min-width: 80px;--spectrum-textfield-m-min-width:0; width:160px;" | ||
| > | ||
| <span slot="field-label">Some fruits are disabled (light DOM)</span> |
There was a problem hiding this comment.
Using the label slot
| @property({ type: String, reflect: true, attribute: 'side-aligned' }) | ||
| public sideAligned?: 'start' | 'end'; | ||
|
|
||
| public renderFieldLabel(fieldId?: string): TemplateResult { |
There was a problem hiding this comment.
renders the label in the shadowDOM of the element
| } = {}): TemplateResult => { | ||
| return html` | ||
| <sp-field-label for="name" size=${ifDefined(size)}> | ||
| <sp-number-field size=${ifDefined(size)} value="100"> |
There was a problem hiding this comment.
Number field uses TextfieldBase which now includes the mixin for a slotted label
|
|
||
| ```html | ||
| <sp-color-field value="#ffff00"></sp-color-field> | ||
| <sp-color-field value="#ffff00">Background color</sp-color-field> |
There was a problem hiding this comment.
Color field uses TextfieldBase which now includes the mixin for a slotted label
| `; | ||
| } | ||
|
|
||
| protected renderPlaceholderContent(): TemplateResult { |
There was a problem hiding this comment.
I separated this into its own function so it would be easier to override the render for a Picker.
| `; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I needed this to make it easier to override for picker without completely overriding the render. In future we should reconsider whether we should have a PickerBase that can be used for menus have different roles and labelling expectations than a picker which is semantically a select-only combobox.
Pull Request Test Coverage Report for Build 18572166522Details
💛 - Coveralls |
4 participants
Description
Motivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review