-
Notifications
You must be signed in to change notification settings - Fork 75
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.
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
fix(pagination): use semantic internal elements #9479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from a few comments, this LGTM!
> | ||
<calcite-icon flipRtl icon={ICONS.previous} scale={getIconScale(this.scale)} /> | ||
</button> | ||
<li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you extract the list item rendering to a separate function and reuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could create a functional component for a li
but that seems a little overkill for this since its just a list item with a class.
packages/calcite-components/src/components/pagination/pagination.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/pagination/pagination.e2e.ts
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/pagination/pagination.e2e.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
const page = await newE2EPage(); | ||
await page.setContent(`<calcite-pagination total-items="10" page-size="1"></calcite-pagination>`); | ||
const list = await page.find(`calcite-pagination >>> .${CSS.list}`); | ||
expect(list).not.toBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also assert on the element types? Maybe we can the following test util:
function assertElementTag(element: E2EElement, tag: keyof HTMLElementTagNameMap): void {
return expect(element.tagName.toLowerCase()).toBe(tag);
}
Related Issue: #7804
Summary