Skip to content

fix(list): remove border from last item with item-sliding #28439

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Nov 3, 2023
Merged

fix(list): remove border from last item with item-sliding #28439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
55feb0c
test: add failing test
liamdebeasi Oct 31, 2023
49a3d34
remove only
liamdebeasi Oct 31, 2023
2aff234
fix(list): remove border from last item with item-sliding
liamdebeasi Oct 31, 2023
c11f03a
test: check for start options too
liamdebeasi Oct 31, 2023
169a75e
fix: more robust fix
liamdebeasi Oct 31, 2023
c5d9418
add another test
liamdebeasi Oct 31, 2023
9a45cde
chore(): add updated snapshots
Ionitron Oct 31, 2023
56247db
cleanup
liamdebeasi Oct 31, 2023
2d264db
clean up
liamdebeasi Oct 31, 2023
21a5ba9
Update core/src/components/list/list.ios.scss
liamdebeasi Oct 31, 2023
3a63aec
Update core/src/components/list/list.md.scss
liamdebeasi Oct 31, 2023
8c1989b
Update core/src/components/list/list.md.scss
liamdebeasi Oct 31, 2023
7a00cfc
add annotations
liamdebeasi Oct 31, 2023
127e374
fix typos
liamdebeasi Nov 1, 2023
1c83d15
fix: only item in list has radius on all corners
liamdebeasi Nov 1, 2023
448b88c
add comment
liamdebeasi Nov 1, 2023
057988c
Update list.md.scss
liamdebeasi Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions core/src/components/list/list.ios.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,25 @@
@include border-radius($list-inset-ios-border-radius);
}

.list-ios.list-inset ion-item:last-child {
/**
* These selectors ensure the last item in the list
* has the correct border.
* We need to consider the following scenarios:
1. The only item in a list.
2. The last item in a list as long as it is not the only item.
3. The item in the last item-sliding in a list.
* Note that we do not select ion-item:last-of-type
* because that will cause the borders to disappear on
* items in an item-sliding when the item is the last
* element in the item-sliding container.
*/
.list-ios.list-inset ion-item:only-child,
.list-ios.list-inset ion-item:not(:only-of-type):last-of-type,
.list-ios.list-inset ion-item-sliding:last-of-type ion-item {
--border-width: 0;
--inner-border-width: 0;
}


.list-ios.list-inset + ion-list.list-inset {
@include margin(0, null, null, null);
}
Expand Down
44 changes: 41 additions & 3 deletions core/src/components/list/list.md.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,50 @@
@include border-radius($list-inset-md-border-radius);
}

.list-md.list-inset ion-item:first-child {
/**
* These selectors ensure the first item in the list
* has the correct radius.
* We need to consider the following scenarios:
1. The first item in a list as long as it is not the only item.
2. The item in the first item-sliding in a list.
* Note that we do not select "ion-item-sliding ion-item:first-of-type"
* because that will cause the borders to disappear on
* items in an item-sliding when the item is the first
* element in the item-sliding container.
*/
.list-md.list-inset ion-item:not(:only-of-type):first-of-type,
.list-md.list-inset ion-item-sliding:first-of-type ion-item {
--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};
/**
* These selectors ensure the last item in the list
* has the correct radius and border.
* We need to consider the following scenarios:
1. The last item in a list as long as it is not the only item.
2. The item in the last item-sliding in a list.
* Note that we do not select "ion-item-sliding ion-item:last-of-type"
* because that will cause the borders to disappear on
* items in an item-sliding when the item is the last
* element in the item-sliding container.
*/
.list-md.list-inset ion-item:not(:only-of-type):last-of-type,
.list-md.list-inset ion-item-sliding:last-of-type ion-item {
--border-radius: #{0 0 $list-inset-md-border-radius $list-inset-md-border-radius};
--border-width: 0;
--inner-border-width: 0;
}

/**
* The only item in a list should have a border radius
* on all corners.
* We target :only-child instead of :only-of-type
* otherwise borders will disappear on items inside of
* ion-item-sliding because the item will be the only
* one of its type inside of the ion-item-sliding group.
*/
.list-md.list-inset ion-item:only-child {
--border-radius: #{$list-inset-md-border-radius};
Copy link
Member

@brandyscarney brandyscarney Nov 1, 2023 •
edited
Loading

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:

ionic-framework/core/src/components/list/list.md.scss

Lines 26 to 33 in a455147

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

--border-width: 0;
--inner-border-width: 0;
}
Expand Down
159 changes: 159 additions & 0 deletions core/src/components/list/test/lines/list.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,162 @@ configs().forEach(({ title, screenshot, config }) => {
});
});
});

/**
* Padding and border color ensures the bottom border can be easily seen if it regresses.
* The background color ensures that any border radius values can be seen.
*/
configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
test.describe(title('list: lines with children'), () => {
test('only item in inset list should not have line', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28435',
});
await page.setContent(
`
<style>
#container {
padding: 10px;
background: #0088cc;
}

ion-item {
--border-color: red;
}
</style>
<div id="container">
<ion-list inset="true">
<ion-item>
<ion-label>Item 0</ion-label>
</ion-item>
</ion-list>
</div>
`,
config
);

const container = page.locator('#container');

await expect(container).toHaveScreenshot(screenshot('inset-list-only-item-no-lines'));
});
test('last item in inset list with end options should not have line', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28435',
});
await page.setContent(
`
<style>
#container {
padding: 10px;
background: #0088cc;
}

ion-item {
--border-color: red;
}
</style>
<div id="container">
<ion-list inset="true">
<ion-item-sliding>
<ion-item>
<ion-label>Item 0</ion-label>
</ion-item>
<ion-item-options slot="end">
<ion-item-option color="warning">
<ion-icon slot="icon-only" name="pin"></ion-icon>
</ion-item-option>
</ion-item-options>
</ion-item-sliding>

<ion-item-sliding>
<ion-item>
<ion-label>Item 1</ion-label>
</ion-item>
<ion-item-options slot="end">
<ion-item-option color="warning">
<ion-icon slot="icon-only" name="pin"></ion-icon>
</ion-item-option>
</ion-item-options>
</ion-item-sliding>

<ion-item-sliding>
<ion-item>
<ion-label>Item 2</ion-label>
</ion-item>
<ion-item-options slot="end">
<ion-item-option color="warning">
<ion-icon slot="icon-only" name="pin"></ion-icon>
</ion-item-option>
</ion-item-options>
</ion-item-sliding>
</ion-list>
</div>
`,
config
);

const container = page.locator('#container');

await expect(container).toHaveScreenshot(screenshot('inset-list-end-options-no-lines'));
});
test('last item in inset list with start options should not have line', async ({ page }) => {
await page.setContent(
`
<style>
#container {
padding: 10px;
background: #0088cc;
}

ion-item {
--border-color: red;
}
</style>
<div id="container">
<ion-list inset="true">
<ion-item-sliding>
<ion-item-options slot="start">
<ion-item-option color="warning">
<ion-icon slot="icon-only" name="pin"></ion-icon>
</ion-item-option>
</ion-item-options>
<ion-item>
<ion-label>Item 0</ion-label>
</ion-item>
</ion-item-sliding>

<ion-item-sliding>
<ion-item-options slot="start">
<ion-item-option color="warning">
<ion-icon slot="icon-only" name="pin"></ion-icon>
</ion-item-option>
</ion-item-options>
<ion-item>
<ion-label>Item 1</ion-label>
</ion-item>
</ion-item-sliding>

<ion-item-sliding>
<ion-item-options slot="start">
<ion-item-option color="warning">
<ion-icon slot="icon-only" name="pin"></ion-icon>
</ion-item-option>
</ion-item-options>
<ion-item>
<ion-label>Item 2</ion-label>
</ion-item>
</ion-item-sliding>
</ion-list>
</div>
`,
config
);

const container = page.locator('#container');

await expect(container).toHaveScreenshot(screenshot('inset-list-start-options-no-lines'));
});
});
});
Binary file added ...2e.ts-snapshots/inset-list-end-options-no-lines-ios-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...e.ts-snapshots/inset-list-end-options-no-lines-ios-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...2e.ts-snapshots/inset-list-end-options-no-lines-ios-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...e2e.ts-snapshots/inset-list-end-options-no-lines-md-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...2e.ts-snapshots/inset-list-end-options-no-lines-md-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...e2e.ts-snapshots/inset-list-end-options-no-lines-md-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....e2e.ts-snapshots/inset-list-only-item-no-lines-ios-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...e2e.ts-snapshots/inset-list-only-item-no-lines-ios-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....e2e.ts-snapshots/inset-list-only-item-no-lines-ios-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...t.e2e.ts-snapshots/inset-list-only-item-no-lines-md-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....e2e.ts-snapshots/inset-list-only-item-no-lines-md-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...t.e2e.ts-snapshots/inset-list-only-item-no-lines-md-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....ts-snapshots/inset-list-start-options-no-lines-ios-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...ts-snapshots/inset-list-start-options-no-lines-ios-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....ts-snapshots/inset-list-start-options-no-lines-ios-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...e.ts-snapshots/inset-list-start-options-no-lines-md-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....ts-snapshots/inset-list-start-options-no-lines-md-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...e.ts-snapshots/inset-list-start-options-no-lines-md-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.