Skip to content

Conversation

@mapsandapps
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

item-divider has padding on mode ios but not on md

Screenshot 2023-03-24 at 11 16 59 AM

Issue URL: #23785

What is the new behavior?

  • item-divider has padding on both modes: ios and md

Screenshot 2023-03-24 at 11 17 13 AM

Does this introduce a breaking change?

  • Yes
  • No

Other information

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label Mar 24, 2023
@mapsandapps mapsandapps marked this pull request as ready for review March 24, 2023 19:44
@mapsandapps mapsandapps requested a review from a team as a code owner March 24, 2023 19:44
@brandyscarney
Copy link
Member

The code fix looks good, but I am wondering if we should simplify the item divider tests so it is just a screenshot of the item divider itself instead of an entire page. Thoughts, @liamdebeasi?

@liamdebeasi
Copy link
Contributor

I think that's a good idea. Smaller screenshots make it easier to identify what changed and will speed up test runs.

@brandyscarney
Copy link
Member

@mapsandapps Can you update the item divider tests to add screenshots of only the divider? We can do something like this:

item-divider/test/basic/item-divider.e2e.ts:

import { expect } from '@playwright/test';
import { test } from '@utils/test/playwright';

test.describe('item-divider: basic', () => {
  test('should display an item divider with text', async ({ page, skip }) => {

    await page.setContent(`
      <ion-item-divider>
        <ion-label>Item Divider</ion-label>
      </ion-item-divider>
    `);

    const divider = page.locator('ion-item-divider');
    await expect(divider).toHaveScreenshot(`item-divider-text-${page.getSnapshotSettings()}.png`);
  });

  test('should display an item divider with a button in the end slot', async ({ page, skip }) => {

    await page.setContent(`
      <ion-item-divider>
        <ion-label>Item Divider</ion-label>
        <ion-button slot="end">Button</ion-button>
      </ion-item-divider>
    `);

    const divider = page.locator('ion-item-divider');
    await expect(divider).toHaveScreenshot(`item-divider-button-end-${page.getSnapshotSettings()}.png`);
  });

  test('should display an item divider with an icon in the start slot', async ({ page, skip }) => {

    await page.setContent(`
      <ion-item-divider>
        <ion-icon slot="start" name="star"></ion-icon>
        <ion-label>Item Divider</ion-label>
      </ion-item-divider>
    `);

    const divider = page.locator('ion-item-divider');
    await expect(divider).toHaveScreenshot(`item-divider-icon-start-${page.getSnapshotSettings()}.png`);
  });
});

Feel free to modify these tests or add more, I was just thinking of some coverage we could have for both slots.

You would then need to revert the addition to the spec test and ionitron's commit to update the reference screenshots. Please let me or Liam know if you have questions!

test.describe('item-divider: spec', () => {
test.beforeEach(async ({ page }) => {
await page.goto('/src/components/item-divider/test/spec');
test.describe('item-divider: basic', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I meant put this test in its own file in a directory named basic:

core/src/components/item-divider/test/basic/item-divider.e2e.ts

Then revert the test in the spec/ folder to match main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, cool. from what y'all were saying i thought you wanted smaller tests, but this way there are more tests. is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think the spec test is needed to be taken as a whole page screenshot in order to check how the divider lines up with the items, we can revisit this when we change the test generator unless we want to strip the spec test to just a couple item dividers + items now.

@brandyscarney
Copy link
Member

Can you revert the file changes to the item-divider/spec screenshots? You might need to check them out from main and then commit them:

git checkout origin/main src/components/item-divider/test/spec/item-divider.e2e.ts-snapshots/

I had to do that on another PR because reverting the commit didn't work.

After that you'll want to run the update reference screenshots action on your branch to generate the new basic item divider screenshots.

@mapsandapps mapsandapps merged commit 426913d into main Mar 31, 2023
@mapsandapps mapsandapps deleted the FW-140 branch March 31, 2023 21:00
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.

5 participants