Skip to content

fix(select): slotted label content works with outline notch #27483

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

Merged
merged 29 commits into from
May 24, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented May 15, 2023

Issue number: N/A


What is the current behavior?

The label slot content does not work with the notch behavior for fill="outline". The notch cutout does not match the dimensions of the floating/stacked label.

What is the new behavior?

  • ion-select now computes the notch size manually if slotted label content is used.

Stencil re-renders the component when slotted content is added, removed, or modified, so we call setNotchWidth in componentDidRender. This ensures that the method is run if labelPlacement is changed too.

setNotchWidth has a couple optimizations built in to ensure we are not doing unnecessary computations:

  1. If no label exists or the label prop is in use then the function returns early. The notch for the label prop is computed based on its intrinsic size because we can render that text content in multiple places.
  2. If the slotted label has a width of 0 but has an offsetParent then we return early to avoid adding an IntersectionObserver on content that is not hidden.

In the event that the select is hidden, then we use an IntersectionObserver to be notified of when the select is visible again. At that point, we will attempt to set the notch width again.

Does this introduce a breaking change?

  • Yes
  • No

Other information

liamdebeasi and others added 14 commits May 12, 2023 14:44
)

Issue number: resolves #27201

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

Selects with a floating label, no value, and a placeholder should have
the label cover the placeholder when blurred. One focus, the label
should translate to the top of the select, and the placeholder should be
visible.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Floating label now covers the select and hides the placeholder when
the select is blurred, matching the `ion-input` and `ion-textarea`
behaviors.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: ionitron <hi@ionicframework.com>
@stackblitz
Copy link

stackblitz bot commented May 15, 2023

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

@github-actions github-actions bot added the package: core @ionic/core package label May 15, 2023
@liamdebeasi liamdebeasi changed the title 3532 notch fix(select): slotted label content works with outline notch May 16, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review May 16, 2023 21:32
@liamdebeasi liamdebeasi requested a review from a team as a code owner May 16, 2023 21:32
@sean-perkins sean-perkins requested a review from a team May 17, 2023 03:50
`
<style>
.custom-label {
font-size: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting this style on a floating label causes the label to render too low when the select is closed:

weird placement

Not sure if this is best addressed in another PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another tricky one. The problem here is we are translating based on %, which will be based on the height of the label element. If the label element grows in height so will the translation.

MDC "fixes" this by giving the label a fixed height. However, this causes the text to get cut off at larger font sizes. This also causes the label to no longer be centered in the container as the select increases in height, though I'm not 100% if that's by design.

We could also make the label height 100%, but then we're going to need to translate by seemingly arbitrary values that may also be wrong as the select changes in height.

Any thoughts on how best to address? I'll keep testing it. It's worth noting that Material Design doesn't seem to do a lot of customizations on floating/stacked labels beyond colors and such. Perhaps this is by design?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to get pretty close by changing the following styles on .label-text-wrapper (screenshots linked):

  • In the default styles, replace the current transform with top: 50% and transform: translateY(-50%) scale(1). The top: 50/translateY(-50) is a trick I've used to vertically center absolutely positioned items.
  • When floating, add top: auto.

The animation doesn't work right, but maybe there's a good way to fix it? An alternative could be to not do position: absolute at all and use flex styles to vertically center the label instead, maybe only when the label isn't floating if the position is necessary to make the float work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During refinement we discussed that this is the same problem as https://ionic-cloud.atlassian.net/browse/FW-4083, so we are going to handle this fix in a separate ticket.

@liamdebeasi liamdebeasi merged commit 436320b into FW-3532 May 24, 2023
@liamdebeasi liamdebeasi deleted the 3532-notch branch May 24, 2023 12:51
Sign up for free to 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.

4 participants