Skip to content

feat(select): add label slot - #27545

Merged
liamdebeasi merged 8 commits into
feature-7.1from
FW-3532
May 25, 2023
Merged

feat(select): add label slot#27545
liamdebeasi merged 8 commits into
feature-7.1from
FW-3532

Conversation

@liamdebeasi

@liamdebeasiliamdebeasi commented May 24, 2023

Copy link
Copy Markdown
Contributor

Issue number: resolves#26838


What is the current behavior?

Developers are unable to pass custom HTML as a label to ion-select.

What is the new behavior?

  • Adds a label slot to ion-select so developers can pass custom HTML for the label.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Docs PR: ionic-team/ionic-docs#2971

@bolt-new-by-stackblitz

Copy link
Copy Markdown

Review PR in StackBlitz CodeflowRun & review this pull request in StackBlitz Codeflow.

@github-actionsgithub-actionsBot added the package: core @ionic/core package label May 24, 2023
@liamdebeasiliamdebeasi changed the title Fw 3532feat(select): add label slotMay 24, 2023
@liamdebeasi
liamdebeasi marked this pull request as ready for review May 24, 2023 14:29
@liamdebeasi
liamdebeasi requested a review from a team as a code ownerMay 24, 2023 14:29
labelText = label ? label.textContent : null;
} else {
labelText = this.label;
labelText = this.labelText;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be this.labelText()?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this.labelText is a getter which binds the property to a function. When you look up this.labelText the function is automatically called: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

Comment threadcore/src/components/select/select.tsx
Comment threadcore/src/components/select/select.tsx
@thetaPC
thetaPC requested a review from a teamMay 24, 2023 17:02

@thetaPCthetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thetaPC
thetaPC requested a review from a teamMay 24, 2023 17:22
Comment threadcore/src/components/select/select.tsx Outdated
Comment on lines +785 to +796
/**
* If no label is being used, then we
* do not need to estimate the notch width.
*/
!this.hasLabel ||
/**
* If the label property is being used
* then we can render the label text inside
* of the notch and let the browser
* determine the notch size for us.
*/
this.label !== undefined

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: As far as the label checks go, !this.hasLabel || this.label !== undefined is equivalent to this.label === undefined || this.label !== undefined, which is always true, so you could combine these checks into this.labelSlot === null. I can understand wanting to black-box the hasLabel logic in case it changes in the future, though, so feel free to leave this as-is.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concern I have with this change is it does not consider if both the label slot and prop are defined. If we only checked this.labelSlot === null then the code would attempt to set the notch width if both the label slot and prop are defined.

However, I could change the check to something like this:

this.label===undefined&&this.labelSlot!==null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true! Sure, that works for me.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in ccc3b60

liamdebeasiand others added 3 commits May 25, 2023 11:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core@ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@liamdebeasi@thetaPC@averyrousseau