Skip to content
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

Jump to bottom

feat(pagination): enable responsive layout #7722

Merged
merged 32 commits into from
Sep 18, 2023
Merged

feat(pagination): enable responsive layout #7722

merged 32 commits into from
Sep 18, 2023

Conversation

driskull
Copy link
Member

@driskull driskull commented Sep 11, 2023 •
edited
Loading

Related Issue: #6687

Summary

  • Makes pagination responsive
  • Uses resize observer
  • Makes ellipsis and page styles equivalent
  • Refactor page rendering logic to count ellipsis as well as pages. Next/previous buttons are not counted because they are always present.
  • Exports Breakpoints interface from helper
  • Modifies some e2e tests
  • Adds screenshot tests

These are the proposed amount of items per breakpoint. (item numbers are pages + ellipsis)

const maxItemBreakpoints = {
  large: 11,
  medium: 9,
  small: 7,
  xsmall: 5,
};

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Sep 11, 2023
@driskull driskull marked this pull request as ready for review September 11, 2023 20:54
@driskull driskull requested a review from a team as a code owner September 11, 2023 20:54
@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 11, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 11, 2023
jcfranco
jcfranco approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Aside from some cleanup comments, this LGTM!

+@SkyeSeitz for design review.

packages/calcite-components/src/components/pagination/pagination.stories.ts Outdated
</div>`;
};

export const responsiveSmall = (): string => breakpoints.map((width) => getResponsiveTemplate(width, "s")).join("");
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, I like this very much. We could add a util to make it easier to create all the necessary breakpoint stories across components.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah a util would be nice here 👍

Copy link
Member

Choose a reason for hiding this comment

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

Taking a stab at this to help with the rest of the responsive component work.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool. I think ill wait for it to be merged to update it here.

packages/calcite-components/src/utils/responsive.ts
@@ -17,6 +17,8 @@ function breakpointTokenToNumericalValue(style: CSSStyleDeclaration, tokenName:
* This util will return a breakpoints lookup object.
*
* Note that the breakpoints will be evaluated at the root and cached for reuse.
*
* @returns {Promise<Breakpoints>} The Breakpoints object.
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were no longer adding types via JSDoc and relying solely on TypeScript. cc @benelan

Copy link
Member Author

Choose a reason for hiding this comment

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

My linter always tells me to do this and I do not argue.

Copy link
Member

@jcfranco jcfranco Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Always wise not to argue with linters. 🤜💥

Serious talk, we disabled jsdoc/require-param-type some time ago, so I'm wondering if this linting error is coming from somewhere else (maybe a VSCode plugin)? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Thats what I see. I've done npm install on the root.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco do we have require-returns disabled?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I was about to disable jsdoc/require-returns-type but it looks like Matt beat me to it. Should we also disable jsdoc/require-property-type while we are at it?

packages/calcite-components/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
@jcfranco jcfranco requested a review from SkyeSeitz September 14, 2023 18:04
@driskull driskull changed the title feat(pagination): enable responsive layout feat(pagination): enable responsive layout [WIP] Sep 14, 2023
@driskull driskull removed the request for review from SkyeSeitz September 14, 2023 19:38
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 14, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 16, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 18, 2023
jcfranco
jcfranco approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Code LGTM!

@SkyeSeitz Can you confirm the following is a part of the required changes?

Makes component flex:1 to expand to available width.

This will affect existing consumers that aren't expecting full width in their usage.

packages/calcite-components/src/components/pagination/pagination.stories.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/pagination/pagination.tsx Show resolved Hide resolved
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 18, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 18, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 18, 2023
@driskull
Copy link
Member Author

Makes component flex:1 to expand to available width.

Reverted this.

@SkyeSeitz
Copy link

Code LGTM!

@SkyeSeitz Can you confirm the following is a part of the required changes?

Makes component flex:1 to expand to available width.

This will affect existing consumers that aren't expecting full width in their usage.

Thanks, Franco. We discussed and we found it does not need the flex:1 behavior. Matt's updates all look good to me! 🚀

@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 18, 2023
@driskull driskull merged commit 933a910 into main Sep 18, 2023
14 checks passed
@driskull driskull deleted the dris0000/pagination-responsive-feat branch September 18, 2023 23:47
@calcite-admin calcite-admin mentioned this pull request Sep 18, 2023
Merged
geospatialem pushed a commit that referenced this pull request Oct 3, 2023
@calcite-admin @github-actions
chore: release main (#7718)
c6f0f81 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.9.0</summary>

##
[1.9.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-10-03)


### Features

* **action-group:** Add css custom properties to define gap and padding
when layout is "grid"
([#7763](#7763))
([b9bd0de](b9bd0de))
* **action-menu:** Add appearance property to configure trigger
appearance
([#7867](#7867))
([36ceaf1](36ceaf1))
* **alert:** Make component responsive
([#7755](#7755))
([66222b5](66222b5))
* **block, input-time-picker, notice:** Adds `open`, `close`,
`beforeOpen`, and `beforeClose` events for consistent event handling
([#7494](#7494))
([04ce758](04ce758))
* **checkbox:** Add WCAG AA recommended minimum 24px square hotspot
optimized for touch users
([#7773](#7773))
([f13e2c4](f13e2c4))
* **flow, flow-item:** Allow calciteFlowItemBack event to be cancelled
([#7855](#7855))
([b74b4df](b74b4df))
* **input-date-picker, input-time-picker:** Add status property
([#7915](#7915))
([4d53346](4d53346))
* **input-time-zone:** Add max-items support
([#7705](#7705))
([c698c27](c698c27))
* **input-time-zone:** Add time zone offset and name mode
([#7913](#7913))
([80bd6ae](80bd6ae))
* **list:** Add newIndex and oldIndex event details to
calciteListOrderChange event
([#7874](#7874))
([0d5cc20](0d5cc20))
* **pagination:** Enable responsive layout
([#7722](#7722))
([933a910](933a910))
* **panel, flow-item:** Add support for collapsing content
([#7857](#7857))
([855754d](855754d))


### Bug Fixes

* **action-bar:** Improve overflowing horizontal actions.
([#7877](#7877))
([52f2d2a](52f2d2a))
* **action-bar:** Overflow actions when overflowActionsDisabled is set
to false
([#7873](#7873))
([3dcceb0](3dcceb0))
* **action-menu:** Update active selection to not use the action's
active property
([#7911](#7911))
([50f85f1](50f85f1))
* **alert:** Regression fix to restore `calciteAlertBeforeOpen` and
`calciteAlertOpen` event emitting
([#7767](#7767))
([6bbae35](6bbae35))
* **button:** Provides context for AT users when used as reference
element for collapsible content
([#7658](#7658))
([50cb3a6](50cb3a6))
* **combobox:** Clears input value on blur
([#7721](#7721))
([e04cc4e](e04cc4e))
* **combobox:** Fix filtering to avoid showing unrelated items
([#7704](#7704))
([b6d32f3](b6d32f3))
* **dropdown-group:** Set selectionMode on slotted dropdown-item
children
([#7897](#7897))
([aa0731a](aa0731a))
* **dropdown:** Support dispatching enter key event on dropdown trigger
([#7752](#7752))
([ba92463](ba92463))
* **select:** Allow setting an option value to an empty string
([#7769](#7769))
([adca6ec](adca6ec))
* **stepper:** Improves AT Users experience with screen readers
([#7691](#7691))
([071dec7](071dec7))
* **tab-nav:** Update indicator position when tab icon changes
([#7768](#7768))
([cb877b3](cb877b3))
* **table:** Fix selection display in RTL
([#7907](#7907))
([b4c8508](b4c8508))
* **tree:** Avoid modifying selected items for "none" selection-mode
([#7904](#7904))
([0bd4a12](0bd4a12))
* **tree:** Fix tree keyboard selection issue
([#7908](#7908))
([53d6c12](53d6c12))
* **tree:** Update tree selection per design spec
([#7852](#7852))
([06b3594](06b3594))
</details>

<details><summary>@esri/calcite-components-react: 1.9.0</summary>

##
[1.9.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-10-03)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize undefined versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.9.0-next.18 to ^1.9.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benelan benelan benelan left review comments

@SkyeSeitz SkyeSeitz SkyeSeitz approved these changes

@jcfranco jcfranco jcfranco approved these changes

@macandcheese macandcheese Awaiting requested review from macandcheese

Assignees

@driskull driskull

Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2023 September Priorities - Main release
Development

Successfully merging this pull request may close these issues.
4 participants
@driskull @SkyeSeitz @jcfranco @benelan