Skip to content

test(breadcrumbs): check color attribute renders in the DOM - #27080

Merged
thetaPC merged 7 commits into
mainfrom
FW-3889
Apr 3, 2023
Merged

test(breadcrumbs): check color attribute renders in the DOM#27080
thetaPC merged 7 commits into
mainfrom
FW-3889

Conversation

@thetaPC

Copy link
Copy Markdown
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?

The test was only running for Vue.

Issue URL: N/A (requested via comment)

What is the new behavior?

  • Runs in the core

Does this introduce a breaking change?

  • Yes
  • No

Other information

N/A

@thetaPC
thetaPC requested a review from a team as a code ownerMarch 31, 2023 21:35
@bolt-new-by-stackblitz

Copy link
Copy Markdown

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

@github-actionsgithub-actionsBot added the package: core @ionic/core package label Mar 31, 2023
@github-actionsgithub-actionsBot added the package: vue @ionic/vue package label Mar 31, 2023
description: 'https://github.com/ionic-team/ionic-framework/issues/25446',
});

const breadcrumbs = page.locator('#color');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to use a unit test here instead of an e2e test?

It looks like we only need to check the output in the DOM, and unit tests run much faster than e2e tests.

You could do something like this:

it('should have color attribute',async()=>{constpage=awaitnewSpecPage({components: [Breadcrumb,Breadcrumbs],html: ` <ion-breadcrumbs color="danger"> <ion-breadcrumb>First</ion-breadcrumb> </ion-breadcrumbs> `});constbreadcrumbs=page.body.querySelector('ion-breadcrumbs');expect(breadcrumbs.hasAttribute('color')).toBe(true);});

We use Stencil's unit testing library: https://stenciljs.com/docs/unit-testing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be able to place this in src/components/breadcrumbs/test/breadrumbs.spec.ts

@thetaPC
thetaPC requested a review from liamdebeasiApril 3, 2023 17:36

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple small changes, but otherwise this looks great. Thanks for taking care of this!

<h1>Color: Primary</h1>

<ion-breadcrumbs color="primary">
<ion-breadcrumbs id="color" color="primary">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good catch, thank you!

Comment on lines +5 to +9
test.beforeEach(async ({ page }) => {
await page.goto('/src/components/breadcrumbs/test/basic');
});

test('should not have visual regressions', async ({ page }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we revert this if it's not needed?

@thetaPC
thetaPC merged commit f866783 into mainApr 3, 2023
@thetaPC
thetaPC deleted the FW-3889 branch April 3, 2023 18:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core@ionic/core packagepackage: vue@ionic/vue package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@thetaPC@liamdebeasi