-
Notifications
You must be signed in to change notification settings - Fork 236
feat(field-label): added and implemented field-label mixin #5799
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
Conversation
feat(label): init field label mixin
🦋 Changeset detectedLatest commit: f24e789 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 |
Tachometer resultsChromemenu permalinktest-basic
Firefoxmenu permalinktest-basic
|
Co-authored-by: Marissa Huysentruyt <[email protected]>
Co-authored-by: Marissa Huysentruyt <[email protected]>
| * @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm.... this is interesting... Textfield already has FieldLabelMixin applied in its inheritance chain....
Textfield → TextfieldBase → FieldLabelMixin(ManageHelpText(...))
I'm not sure, but this sounds like this it could create problems? Can't say for sure what, but could this lead to duplicate field-label styles?, two separate renderFieldLabel() implementations where one shadows the other?, potentially conflicting slot observation? 🤷
Maybe the solution is just
| export class Combobox extends FieldLabelMixin(Textfield, 'field-label') { | |
| export class Combobox extends Textfield { |
and keep the renderFieldLabel('input') that we have below. Let me know what you think!
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.
TL;DR, we need this for now but it will be gone before the merge to main
field-label is the slotName not the fieldId. In text field it makes sense just to make the slotName a default slot, but in combobox, the menuitems are in the default slot.
Also, TBH, the minute I touch SWC-710, which is also part of this branch, combobox will not extend textfield, so this is only temporary, and will be changed before the parent branch gets merged to main.
By the time this merges to main, it will be export class Combobox extends FieldLabelMixin(Menu, 'field-label') { because we need to move the menu to the same DOM as everything else and there is more I need to inherit from Menu than I do from a Textfield.
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'm happy with that! Thanks for the context!
Co-authored-by: Rúben Carvalho <[email protected]>
Co-authored-by: Rúben Carvalho <[email protected]>
Co-authored-by: Rúben Carvalho <[email protected]>
Co-authored-by: Rúben Carvalho <[email protected]>
Co-authored-by: Rúben Carvalho <[email protected]>
Co-authored-by: Rúben Carvalho <[email protected]>
packages/textfield/src/Textfield.ts
Outdated
| ) { | ||
| public static override get styles(): CSSResultArray { | ||
| return [textfieldStyles, checkmarkStyles]; | ||
| return [super.styles || [], textfieldStyles, checkmarkStyles]; |
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 if we don'tt spread super.styles, it can be problematic? We can end up with a nested array?
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.
Fun fact:
In our other components, super.styles has a CSSResultArray type. In Textfield, it is CSSResultGroup | undefined.
According to Lit docs:
CSSResultGroupisCSSResultOrNative | CSSResultArray.CSSResultArrayisArray<CSSResultOrNative | CSSResultArray>
In order to get around this, I had to do the following:
const styles = (super.styles || []) as CSSResultArray;
return [...styles, textfieldStyles, checkmarkStyles];* feat(label): init field label mixin feat(label): init field label mixin * feat(textfield): updated textfiled docs to use label mixin * feat(field-label): added mixin to package exports * feat(field-label): added optional slot name for rendering label slot * docs(textfield): implemented label mixin in stories * test(textfield): added test for slotted label * feat(textfield): added default slot observer for label changes * test(textfield): added tests for slotted label * docs(number-field): updated number field docs based on text field label changes * docs(number-field): updated stories with slotted label * docs(color-field): updated docs with slotted label * feat(field-label): updated interface to include optional slot name for label * feat(combobox): added label slot to combobox * docs(combobox): added slotted label to docs * test(combobox): updates tests with slotted label * fix(combobox): fixed combobox field id for label * feat(field-label): added label slot change handler to mixin * feat(combobox): added slot change handlers * feat(field-label): hide field label if not used * feat(textfield): use field label mixin to observe slot * feat(combobox): use field label mixin to observe slot * feat(combobox): added placeholder * fix(textfield): fixed issue with detecting slot content * fix(field-label): fixed issue with slot content detection in text field * test(textfield): removed unneccessary id attribute * fix(fieldl-label): fixed mixin to allow slot naming * feat(combobox): updated combobox, stories and tests for field label mixin * fix(field-label): corrected linting issues * fix(menu): MenuItem focus stealing from input elements on mouseover (#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]> * fix(field-label): made field id optional for picker * fix(field-label): made field id optional for picker * feat(picker): added field label mixin to pciker and picker stories * fix(textfiled): fixed element name in warning * fix(combobox): fixed element name in warning * docs(picker): updated stories * feat(picker): added field label mizin to picker without affecting pickerBase * docs(textfield): documented default label slot * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * feat(picker): reverting changes until picker is refactored * docs(textfield): updated docs with label examples * docs(textfield): fixed issue with tabs * test(textfield): updated test warning text to match revised warning text * docs(combobox): added label and placeholder sections to docs * docs(textfield): changed tabs label * docs(number-field): updated docs with a label section * chore(changeset): added changsets * docs(field-label): added field-label-mixin docs * Apply suggestion from @nikkimk * feat(picker): revert changes * feat(picker): revert changes * chore: updated changesets * docs(color-field): added label docs * chore: updated changesets * docs(combobox): fixed typo Co-authored-by: Casey Eickhoff <[email protected]> * docs(field-label): added JS Docs to mixin * chore: linting fixes * docs(combobox): fixed typo Co-authored-by: Casey Eickhoff <[email protected]> * chore: dropped changeset * docs(color-field): fixed typo Co-authored-by: Marissa Huysentruyt <[email protected]> * chore(combobox): fixed typo * docs(combobox): added side-aligned to story * docs(combobox): fixed typo Co-authored-by: Marissa Huysentruyt <[email protected]> * docs(combobox): fixed typo Co-authored-by: Marissa Huysentruyt <[email protected]> * docs(field-label): added links to mixin docs * docs(field-label): added code type in mixin docs Co-authored-by: Marissa Huysentruyt <[email protected]> * docs(field-label): added code type in mixin docs Co-authored-by: Marissa Huysentruyt <[email protected]> * fix(textfield): simiplified styles getter * fix(textfield): simplified attribute Co-authored-by: Rúben Carvalho <[email protected]> * fix(textfield): simplified attribute * docs(field-label): added more detail to mixin docs Co-authored-by: Rúben Carvalho <[email protected]> * docs(field-label): added more detail to field label mixin docs Co-authored-by: Rúben Carvalho <[email protected]> * fix(field-label): fixed inherited styles in mixin Co-authored-by: Rúben Carvalho <[email protected]> * fix(field-label): set side-aligned property to optional Co-authored-by: Rúben Carvalho <[email protected]> * fix(field-label): added override to styles getter Co-authored-by: Rúben Carvalho <[email protected]> * fix(field-label): field id should be required * fix(field-label): reverted styles changes to mixin * fix(textfield): fixed styles array * fix(textfield): ensure super styles are CSSResultArray --------- Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Casey Eickhoff <[email protected]> Co-authored-by: Marissa Huysentruyt <[email protected]> Co-authored-by: Rúben Carvalho <[email protected]>
Description
Motivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesAll VRTs are approved before the author can update Golden HashNote: Ignore any visual regressions issues at this time as they will be addressed in SWC-1316 before this branch is merged into mainManual review test cases
Note: Ignore any CSS issues at this time as they will be addressed in SWC-1316 before this branch is merged into main
Text Field
Color Field
Number Field
Combobox
Device review