fix(item-divider): removal of unneeded margin unset #27042
Conversation
|
|
thetaPC
changed the title
|
Original PR #27024 was closed since this PR replaces it. The reason was due to changing base. |
There was a problem hiding this comment.
Can we revert this screenshot? Looks to be flaky
| const button = page.locator('ion-button'); | ||
| const buttonMarginEnd = await button.evaluate((el) => { | ||
| const styles = window.getComputedStyle(el); | ||
| return styles.getPropertyValue('margin-end'); | ||
| }); | ||
|
|
||
| expect(buttonMarginEnd).not.toBe('0px'); |
There was a problem hiding this comment.
We should avoid checking exact pixel values in favor of screenshot tests. It looks like test coverage for buttons in dividers was added in 426913d, so I'm not sure we even need this test.
| <ion-item-group> | ||
| <ion-item-divider> | ||
| <ion-label>A</ion-label> | ||
| <ion-button slot="end">My Button</ion-button> |
There was a problem hiding this comment.
We probably don't need this (and the resulting screenshot change) since this check is covered in the tests added in 426913d.
liamdebeasi
left a comment
liamdebeasi
left a comment
There was a problem hiding this comment.
One final thing to look at, and then we're good to go
There was a problem hiding this comment.
Are you able to see why the height of these screenshots are different? I wouldn't expect this to have changed, unless there's a visual diff I'm missing.
There was a problem hiding this comment.
Are you able to see why the height of these screenshots are different? I wouldn't expect this to have changed, unless there's a visual diff I'm missing.
@liamdebeasi I don't know the exact reason either. My guess is that the removal of the unset is causing some minor pixel change. Since the item-fill test has a lot of components then the pixels add up. Thoughts?
There was a problem hiding this comment.
Ok figured it out:
Old
New
The margin is now being applied correctly which causes the text to wrap since the element itself is so narrow.
If we look at the LTR screenshot (which has always applied the margin correctly) we can see the same wrapping: https://github.com/ionic-team/ionic-framework/blob/15decdd0de7427749408f9977f6d1ab2ac969680/core/src/components/item/test/fill/item.e2e.ts-snapshots/item-fill-diff-md-ltr-Mobile-Chrome-linux.png
There was a problem hiding this comment.
In that case, this screenshot change is correct 👍
There was a problem hiding this comment.
Woah, thank you so much!
Pull request checklist
Please check if your PR fulfills the following requirements:
ionic-docsrepo, in a separate PR. See the contributing guide for details.npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
End slots are lacking margin of 2px when in RTL.
Issue URL: Part of #17012
What is the new behavior?
property-horizontalby removing any margin unsetEnd slots with buttons will have a margin-end of 2px regardless of direction.
Screenshots
Does this introduce a breaking change?
Other information