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(css): rtl mixin preserves selectors after :host #25264

Closed
wants to merge 15 commits into from

Conversation

crazyserver
Copy link

@crazyserver crazyserver commented May 10, 2022

Ie: :host .element-selector when using rtl was translated as :host-context([dir=rtl]) instead of :host-context([dir=rtl]) .element-selector

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)
  • 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?

With the following CSS:

:host .element-selector {
    @include position(0, 0, null, null);
}

When using the rtl mixin so, also add-root-selector it translates the selector as:

:host-context([dir=rtl])

So part of it is lost.

Issue URL: #25285

What is the new behavior?

With the following CSS:

:host .element-selector {
    @include position(0, 0, null, null);
}

When using the rtl mixin so, also add-root-selector it translates the selector as:

:host-context([dir=rtl]) .element-selector

Maintaining the whole selector.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Ie: :host .element-selector when using rtl was translated as :host-context([dir=rtl]) instead of :host-context([dir=rtl]) .element-selector
@crazyserver crazyserver requested a review from a team as a code owner May 10, 2022 09:45
@github-actions github-actions bot added the package: core @ionic/core package label May 10, 2022
crazyserver added a commit to crazyserver/moodleapp that referenced this pull request May 10, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the PR! We require that any fix/feature PRs to be associated with an existing issue prior to merging. Is there an open issue that we can link this to?

Having an issue with steps to reproduce helps us better verify the proposed fix.

@crazyserver crazyserver changed the title Fix lost selector when :host is used Fix host selector when :host is used May 13, 2022
@crazyserver
Copy link
Author

Thanks I've opened: #25285

@liamdebeasi liamdebeasi requested review from liamdebeasi, averyjohnston and sean-perkins and removed request for a team May 16, 2022 12:25
@liamdebeasi
Copy link
Contributor

liamdebeasi commented May 16, 2022

Thanks! There are several components that make use of the :host .selector syntax with RTL utilities. These will also need to be updated to account for this change.

For example this line in datetime probably does not need the position mixin: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime.scss#L346

However, the change in this PR breaks its positioning:

image


We will schedule some time in an upcoming sprint to scrutinize which components need updates and make the appropriate changes.

crazyserver added a commit to moodlehq/moodleapp that referenced this pull request May 18, 2022
@liamdebeasi liamdebeasi changed the title Fix host selector when :host is used fix(css): rtl mixin preserves selectors after :host Sep 30, 2022
@sean-perkins sean-perkins requested a review from a team as a code owner November 28, 2022 21:57
@sean-perkins sean-perkins changed the base branch from main to feature-6.4 November 30, 2022 16:57
@github-actions github-actions bot added package: angular @ionic/angular package package: react @ionic/react package package: vue @ionic/vue package labels Nov 30, 2022
@sean-perkins
Copy link
Contributor

Ignore the large file diff for now (getting things ready for team review/merge into 6.4). Once I sync 6.4 with main the diff will be the single file change.

@liamdebeasi liamdebeasi deleted the branch ionic-team:feature-6.4 December 7, 2022 15:52
@liamdebeasi liamdebeasi closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: react @ionic/react package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants