Skip to content
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

fix(item): multiple input appearance when using datetime #25484

Merged
merged 7 commits into from
Jun 16, 2022
Merged

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Jun 15, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Discovered when migrating item tests: #25479

Only in Google Chrome, when using ion-datetime with multiple inputs in an ion-item in RTL mode; the datetime will become clipped.

Issue URL: #25483

What is the new behavior?

  • Explicitly sets the top/left position styles instead of using the RTL utility that incorrectly sets styles on host-context.

Does this introduce a breaking change?

  • Yes
  • No

Other information

You can confirm this change locally with the existing item/test/inputs suite in RTL mode: http://localhost:3333/src/components/item/test/inputs?rtl=true

@sean-perkins sean-perkins requested a review from a team as a code owner June 15, 2022 23:46
@github-actions github-actions bot added the package: core @ionic/core package label Jun 15, 2022
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

I don't think this is the correct way of resolving this issue.

The styles that cause this are being added here: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime.scss#L346

There is a defect in these mixins where the selector is being incorrectly generated, causing the selector to be :host-context([dir=rtl]) instead of :host-context([dir=rtl]) .calendar-day:after. So really, this is just another instance of #25285.

There is a proposed fix in #25264 that should resolve the issue in #25483.

However, it would appear that the fix in https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime.scss#L346 is not actually correct and only worked because of this bug. We will likely need to both these issues at the same time to avoid regressions.

I'll look into a better solution for the .calendar-day:after position fix.

@sean-perkins
Copy link
Contributor Author

Sounds great! Thanks for pointing me to where it's being appended in our code. At one point I was able to narrow it to:

:host .calendar-day:after {
 @include position(50%, null, null, 50%);
}

But didn't consider that the mixin may have been incorrectly applying the styles. Happy to look more into it if other tasks require your time 👍

@liamdebeasi
Copy link
Contributor

Turns out we don't even need the RTL support in https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime.scss#L346. The position+transform styles in the block are fixes for an iOS 13 bug. I tested it out and just doing top: 50% and left: 50% seem to work fine.

We can probably remove the @include and add the top/left for now and add a tech debt ticket to remove the top/left/transform styles when iOS 13 support is dropped.

We should also look into getting #25264 ready for review. However, there are likely more instances of this issue that we have not seen yet. When fixing this it might be good to get a list of affected components. We can probably get a solid list by searching for @include in the code, and then filtering out any selectors that either a) only have :host or b) do not use :host in the selector.

core/src/components/datetime/datetime.scss Outdated Show resolved Hide resolved
core/src/components/datetime/datetime.scss Show resolved Hide resolved
sean-perkins and others added 4 commits June 16, 2022 13:34
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@sean-perkins sean-perkins merged commit 3089f38 into main Jun 16, 2022
sean-perkins added a commit that referenced this pull request Jun 16, 2022
sean-perkins added a commit that referenced this pull request Jun 17, 2022
* Revert "fix(item): multiple input appearance when using datetime (#25484)"

This reverts commit 3089f38.

* fix(item): multiple input appearance when using datetime

Resolves #25483
@liamdebeasi liamdebeasi deleted the FW-1716 branch March 24, 2023 17:58
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.

2 participants