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.

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

Pagination: Single-page behavior, style, and window prop updates #1574

Merged
merged 6 commits into from
Sep 8, 2022

Conversation

philschanely
Copy link
Contributor

@philschanely philschanely commented Sep 1, 2022

Description

This PR updates how Pagination behaves when there is only one page and a few styles to sync with latest spec including:

Screenshots

Task Before After
Updates the disabled style for the "next" and "back" buttons. See Figma spec Screen Shot 2022-09-07 at 3 59 15 PM Screen Shot 2022-09-07 at 3 58 07 PM
Ensures that at least 1 page button is always present when there is only one page of data to show (React) Screen Shot 2022-09-07 at 4 15 54 PM (component hides on Rails) Screen Shot 2022-09-07 at 4 17 09 PM (same for both now)
Ensures a default value for window in Rails and updates documentation accordingly Screen Shot 2022-09-07 at 3 59 23 PM Screen Shot 2022-09-07 at 3 58 36 PM

Testing in sage-lib

See Components > Pagination

Testing in kajabi-products

  1. (Medium) Style and configuration improvements to Pagination including:
    • Clearer disabled buttons
    • When only one page, still show component with one page (improves UX and accessibility)
    • Provides default value for window in Rails version so its now truly optional

Related

SAGE-240 and SAGE-56

- Remove :hover styles where no longer needed
- Update color values
- Add round border radius
@philschanely philschanely self-assigned this Sep 1, 2022
@philschanely philschanely requested review from a team and cameronsimony September 7, 2022 20:52
@philschanely philschanely marked this pull request as ready for review September 7, 2022 20:53
@philschanely philschanely changed the title Pagination: Single-page behavior update Pagination: Single-page behavior, style, and window prop updates Sep 7, 2022
Copy link

@cameronsimony cameronsimony left a comment

Choose a reason for hiding this comment

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

LGTM! 🔥

Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

A couple of tiny comments, but also noticed the React Component link is broken on the Pagination page. Don't believe it's related, but might be worth looking into (could be a follow-up).

Otherwise, LGTM! 👍🏼

@@ -55,6 +55,6 @@
<tr>
<td><%= md('`window`') %></td>
<td><%= md('Sets the number of items on either side of the `...` item. (Note: the left side will be `window` + 1). Has no affect when `hide_pages` is set to true.') %></td>
<td><%= md('String') %></td>
<td><%= md('`nil`') %></td>
<td><%= md('Number') %></td>
Copy link
Member

Choose a reason for hiding this comment

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

Integer, maybe?

<%= component.generated_html_attributes.html_safe %>
>
<% if !component.hide_counter %>
<span class="sage-pagination__count <%= "sage-pagination__count--solo" if component.items.total_pages == 1 %>">
Copy link
Member

Choose a reason for hiding this comment

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

🧛🏼 count solo... made me laugh, def not a show-stopper. sorry back to the serious review.

@pixelflips pixelflips requested a review from a team September 7, 2022 23:33
aria-label="Pagination Navigation"
<%= component.generated_html_attributes.html_safe %>
>
<% if !component.hide_counter %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

preferred in ruby unless component.hide_counter

Copy link
Collaborator

@ju-Skinner ju-Skinner left a comment

Choose a reason for hiding this comment

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

Comments left, do NOT block my approval.

@philschanely philschanely merged commit 4e1e539 into develop Sep 8, 2022
@philschanely philschanely deleted the SAGE-240/pjs-pagination-1p-behavior-sync branch September 8, 2022 17:01
@philschanely philschanely mentioned this pull request Sep 8, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants