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(rtl): allow :host to use rtl() #28353

Merged
merged 9 commits into from
Oct 18, 2023
Merged

fix(rtl): allow :host to use rtl() #28353

merged 9 commits into from
Oct 18, 2023

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Oct 13, 2023

Issue number: N/A


What is the current behavior?

While working on a safe area padding mixin, I realized that rtl() wasn't being applied for :host when using Firefox or Safari. This is happening because the syntax for :dir() is wrong. The placement needs to be updated for Firefox and Safari to register it.

:host {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

:host:dir(rtl) { // <- wrong syntax
   // styles
}
:host(.class) {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  // styles
}

:host-context([dir=rtl]).class {
   // styles
}

:host(.class):dir(rtl) { // <- wrong syntax
   // styles
}

What is the new behavior?

I updated rtl() to use :dir() as the addHostSelector in the add-root-selector function. This generates all the correct selectors for Firefox and Safari. However, :dir() does not have the structure of :host-context() so I had to add a new parameter to add-root-selector to determine whether to use :host-context() or not. I set the default to true since the function originally used :host-context().

An extra win is that the updated function will be ready for when :host-context() can be removed from the codebase.

:host {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

-  :host:dir(rtl) {
+  :host(:dir(rtl)) {
   // styles
}
:host(.class) {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  padding-right: 40px;
}

:host-context([dir=rtl]).class {
   // styles
}

-  :host(.class):dir(rtl) {
+  :host(.class:dir(rtl)) {
   // styles
}

Does this introduce a breaking change?

  • Yes
  • No

Other information

N/A

@github-actions github-actions bot added the package: core @ionic/core package label Oct 13, 2023
@thetaPC thetaPC marked this pull request as ready for review October 16, 2023 14:56
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

I tested it against my PR here where I ran into this issue: #28371

It is working as expected in Firefox but I am still not seeing it resolved in Safari.

Below is the change I see in the generated code for the following code snippet between main and this branch:

.item { 
  @include ltr() {
    padding-left: 40px;
  }

  @include rtl() {
    padding-right: 40px;
  }
}
main branch
.item {
  padding-left: 40px;
}
:host-context([dir=rtl]) .item {
  padding-right: 40px;
}
[dir=rtl] .item {
  padding-right: 40px;
}
@supports selector(:dir(rtl)) {
  .item:dir(rtl) {
    padding-right: 40px;
  }
}
      
.item {
  padding-left: 40px;
}
:host-context([dir=rtl]) .item {
  padding-right: 40px;
}
[dir=rtl] .item {
  padding-right: 40px;
}
@supports selector(:dir(rtl)) {
  :dir(rtl) .item {
    padding-right: 40px;
  }
}
      

Is there something missing?

core/src/themes/ionic.functions.string.scss Outdated Show resolved Hide resolved
@brandyscarney
Copy link
Member

brandyscarney commented Oct 18, 2023

Maria and I looked into the issue in my last comment. It turns out that I was running into this bug while adding ?rtl=true to the browser url of the test I was previewing. If I added dir="rtl" to the html I did not run into this. This bug was fixed in v17 of Safari but I am using v16.6 which is why I was seeing it and Maria was not. She is going to add a comment in the code in case anyone else runs into it while testing.

…into fix-rtl-host

it merges an updated upstream into a topic branch. # # Lines starting with '#' will be
ignored, and an empty message aborts # the commit.
@thetaPC thetaPC requested a review from brandyscarney October 18, 2023 18:21
@thetaPC thetaPC added this pull request to the merge queue Oct 18, 2023
Merged via the queue into main with commit 6b7d288 Oct 18, 2023
@thetaPC thetaPC deleted the fix-rtl-host branch October 18, 2023 21:35
sean-perkins pushed a commit that referenced this pull request Oct 27, 2023
Issue number: N/A

---------

<!-- 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. -->

While working on a safe area padding mixin, I realized that `rtl()`
wasn't being applied for `:host` when using Firefox or Safari. This is
happening because the syntax for `:dir()` is wrong. The placement needs
to be updated for Firefox and Safari to register it.

```scss
:host {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

:host:dir(rtl) { // <- wrong syntax
   // styles
}
```

```scss
:host(.class) {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  // styles
}

:host-context([dir=rtl]).class {
   // styles
}

:host(.class):dir(rtl) { // <- wrong syntax
   // styles
}
```

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

I updated `rtl()` to use `:dir()` as the `addHostSelector` in the
`add-root-selector` function. This generates all the correct selectors
for Firefox and Safari. However, `:dir()` does not have the structure of
`:host-context()` so I had to add a new parameter to `add-root-selector`
to determine whether to use `:host-context()` or not. I set the default
to `true` since the function originally used `:host-context()`.

An extra win is that the updated function will be ready for when
`:host-context()` can be removed from the codebase.


```diff
:host {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

-  :host:dir(rtl) {
+  :host(:dir(rtl)) {
   // styles
}
```

```diff
:host(.class) {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  padding-right: 40px;
}

:host-context([dir=rtl]).class {
   // styles
}

-  :host(.class):dir(rtl) {
+  :host(.class:dir(rtl)) {
   // styles
}
```

## 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. -->

N/A

---------

Co-authored-by: ionitron <[email protected]>
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.

3 participants