Skip to content

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Oct 31, 2023

Issue number: resolves #28435


What is the current behavior?

The item in the last item-sliding still has a border in an inset list.

What is the new behavior?

  • Item in last item-sliding no longer has a border

I originally only added ion-item-sliding:last-of-type ion-item but I discovered that the original ion-item:last-child causes items in item-sliding where the item is the last element in the item-sliding container to not have a border, so the original fix was incomplete.

I added comments as to what each line does and why we didn't just do ion-item:last-child.

Does this introduce a breaking change?

  • Yes
  • No

Other information


Co-authored-by: Brandy Carney brandyscarney@users.noreply.github.com

@github-actions github-actions bot added the package: core @ionic/core package label Oct 31, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review October 31, 2023 17:50
@liamdebeasi liamdebeasi requested review from a team and averyjohnston and removed request for a team October 31, 2023 17:50
liamdebeasi and others added 4 commits October 31, 2023 15:46
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
* on all corners.
*/
.list-md.list-inset ion-item:only-child {
--border-radius: #{$list-inset-md-border-radius};
Copy link
Member

@brandyscarney brandyscarney Nov 1, 2023

Choose a reason for hiding this comment

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

This change seems wrong to me:

main branch
main(Pixel 5) branch(Pixel 5)

Steps to reproduce:

  1. Modify core/src/components/list/test/inset/index.html to:
- <ion-content id="content" color="light">
+ <ion-content id="content" color="tertiary">
+   <ion-list inset>
+     <ion-item>
+       <ion-label>Text Items</ion-label>
+     </ion-item>
+   </ion-list>
+
    <ion-list inset>

Add the following style to the head:

<style>
  ion-item {
    --background: red;
  }
</style>

Increase the border radius on md to see the change better:

$list-inset-md-border-radius:          10px !default;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just following what was there before for border radius:

.list-md.list-inset ion-item:first-child {
--border-radius: #{$list-inset-md-border-radius $list-inset-md-border-radius 0 0};
}
.list-md.list-inset ion-item:last-child {
--border-radius: #{0 0 $list-inset-md-border-radius, $list-inset-md-border-radius};
--border-width: 0;
--inner-border-width: 0;

Demo: https://codepen.io/liamdebeasi/pen/eYxzXjw

Note that the last-child's border radius never showed up because it was formatted incorrectly:
image

(either everything should have a comma, or nothing should)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm okay. I think this might be broken behavior then on md. This code used to be required when the items lined up with the top / bottom of the list but since Material Design added padding to the top and bottom of a list it doesn't line up properly. It looks like we added that in 5 years ago but didn't account for the border radius since item generally has the same background as list.

Material Design 3 also has this padding at the top and bottom of their list. Should we create a tech debt ticket to take a look at this or add it to our MD3 work?

It does look like it should still have this code on ios though. We also don't seem to have any tests verifying this behavior.

@liamdebeasi liamdebeasi enabled auto-merge November 1, 2023 18:05
@liamdebeasi liamdebeasi disabled auto-merge November 1, 2023 18:05
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.

Code looks good, just need to create or update a ticket to take a look at the border radius for items in an inset list on md. 👍

@liamdebeasi liamdebeasi added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit cafafcc Nov 3, 2023
@liamdebeasi liamdebeasi deleted the FW-5503 branch November 3, 2023 14:51
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.

bug: last ion-item-sliding should not have a border-bottom

4 participants