fix(item): apply safe area to proper side regardless of direction - #28403
Merged
Conversation
mapsandapps
marked this pull request as ready for review
October 26, 2023 20:44
mapsandapps
requested review from
averyrousseau and
brandyscarney
as code owners
October 26, 2023 20:44
averyrousseau
approved these changes
Oct 27, 2023
Co-authored-by: Amanda Johnston <90629384+amandaejohnston@users.noreply.github.com>
brandyscarney
requested changes
Oct 27, 2023
brandyscarney
left a comment
brandyscarney
left a comment
Member
There was a problem hiding this comment.
This is looking good, just a couple of things I found that I wanted to bring up!
Member
There was a problem hiding this comment.
I think we might be applying the --ion-safe-area-left to the wrong element here. This might have been a problem prior to this PR but maybe we should look into it?
If you notice on the LTR test the bottom border extends the full width to the right, even though --ion-safe-area-right is set to 10px:
When flipped, the bottom border is not extending fully to the left edge:
Happy to bring this up for team discussion to see if we should make this a separate issue!
Contributor
Author
There was a problem hiding this comment.
I think this was actually a mistake with this PR, not from before. Thanks for catching it!
Contributor
|
Removing myself as a reviewer. I'm getting added as a reviewer due the screenshot changes, but I think Amanda and Brandy have this covered 😄 |
liamdebeasi
removed their request for review
October 30, 2023 14:51
brandyscarney
left a comment
brandyscarney
left a comment
Member
There was a problem hiding this comment.
The code itself looks good, great work! Just some minor feedback on the test.
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
mapsandapps
enabled auto-merge
November 2, 2023 13:31
brandyscarney
approved these changes
Nov 2, 2023
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Nov 2, 2023
2 tasks
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Nov 8, 2023
…tion (#28420) Issue number: Internal --------- <!-- 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. --> 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? <!-- Please describe the behavior or changes that are being added by this PR. --> - 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 - [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> Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com> Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: Internal
What is the current behavior?
The safe area for Item 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?
Note: There is a separate ticket for updating the safe area padding for ion-item-divider.
Does this introduce a breaking change?
Other information