Skip to content

fix(item-divider): apply safe area to proper side regardless of direction - #28420

Merged
mapsandapps merged 8 commits into
mainfrom
FW-5173
Nov 8, 2023
Merged

fix(item-divider): apply safe area to proper side regardless of direction#28420
mapsandapps merged 8 commits into
mainfrom
FW-5173

Conversation

@mapsandapps

@mapsandapps mapsandapps commented Oct 26, 2023

Copy link
Copy Markdown
Contributor

Issue number: Internal


What is the current behavior?

The safe area for Item Divider would change sides when the direction changed. e.g., if the safe area padding should be on the left, it would be on the left when the direction was LTR but would be on the right when the direction was RTL. It should always be on the left.

What is the new behavior?

  • The left safe area padding is on the left side of an Item Divider whether the direction is LTR or RTL.
  • The right safe area padding is on the right side of an Item Divider whether the direction is LTR or RTL.
  • Updated LTR and RTL screenshots for item to include an item divider

Similar to #28403 but for Item Divider.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions Bot added the package: core @ionic/core package label Oct 26, 2023
Comment thread core/src/components/item-divider/item-divider.scss Outdated
Base automatically changed from FW-5172 to main November 2, 2023 14:31
@mapsandapps
mapsandapps marked this pull request as ready for review November 2, 2023 15:22
@mapsandapps
mapsandapps requested review from a team and liamdebeasi and removed request for a team November 2, 2023 15:22

@liamdebeasi liamdebeasi 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.

A few small changes, but otherwise looks good.

Comment thread core/src/components/item-divider/item-divider.scss Outdated
Comment thread core/src/components/item-divider/item-divider.scss Outdated
<div slot="helper">Helper</div>
</ion-item>

<ion-item-divider>

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: I think it's confusing to add the item-divider to the item tests when item-divider has its own test directory in item-divider/test. I'll defer to you and Brandy though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My thinking was

  1. That it would be easier to understand what regression was occurring if Item & Item Divider are in the same test
  2. It's more efficient to combine them into the same test
  3. There are other tests in Item that include Item Dividers

but i can move them. It doesn't really matter. 🤷🏻

mapsandapps and others added 4 commits November 3, 2023 12:57
Comment thread core/src/components/item-divider/test/basic/item-divider.e2e.ts
mapsandapps and others added 2 commits November 8, 2023 09:16
@mapsandapps
mapsandapps added this pull request to the merge queue Nov 8, 2023
Merged via the queue into main with commit 4513e0c Nov 8, 2023
@mapsandapps
mapsandapps deleted the FW-5173 branch November 8, 2023 15:46
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