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

Align pagination buttons with listings column #648

Merged
merged 12 commits into from
Oct 4, 2021

Conversation

anders-schneider
Copy link

@anders-schneider anders-schneider commented Sep 28, 2021

Issue

Description

This change makes it so that the buttons in the bottom pagination bar of the "view all listings" don't extend to the horizontal extrema and instead are aligned with the listings column.

It also removes the border from the pagination bar on the "view all listings" page.

Before:
image

After:
image

This change does not affect the prev/next buttons on the mobile view (where there is no issue with the bottom pagination bar being too wide). In the mobile view, all it does it remove the weird rounded bottom corners:

Before:
image

After:
image

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

  • Desktop View
  • Mobile View
    There are six instances of AgPagination between the public and partner sites. I looked at each in desktop and mobile views.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes
  • I have run yarn generate:client if I made backend changes

@anders-schneider anders-schneider marked this pull request as draft September 28, 2021 21:54
@avaleske
Copy link

hey @anders-schneider we can chat more tomorrow but I like how it looks with your changes!

To your questions, after skimming:

  • Do media queries work for this?
  • Are we able to move the styles to a class and add that class to the dataPagerClassName array, instead of adding the wrapper div?

@anders-schneider anders-schneider marked this pull request as ready for review October 1, 2021 15:31
@anders-schneider
Copy link
Author

Thanks for taking a quick look at this a few days ago @avaleske! It's now in ready-to-actually-be-reviewed shape :)

A few notes:

  • Your suggestion to use media queries was helpful! I'm doing that through the tailwind rules/styles
  • I ended up keeping the structure with a new div inside the outer div. Without that (and instead taking the suggestion of just applying styles on the existing outer div), I couldn't figure out a way to not have weird space on either side of the pagination bar:

image

And two questions:

  • Is pagination-bar an okay name for the new class I've created? It doesn't seem to match the other data-pager classes, but data-pager is less intuitive to me
  • Is ag_grid.scss the right home for this new class? I just put it there because the styles for similar/nearby html elements were in there.

@avaleske
Copy link

avaleske commented Oct 1, 2021

I think there's a regression on mobile, the paginator should look like
image, with the buttons spaced apart.

I think it's an easy fix. testing something one sec.

@anders-schneider
Copy link
Author

Thanks so much @avaleske for your help on chat on this!! I think I've got this in shape for another pass whenever you have time.

Copy link

@avaleske avaleske left a comment

Choose a reason for hiding this comment

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

Thank you!

@anders-schneider anders-schneider merged commit 4a307e9 into main Oct 4, 2021
@anders-schneider anders-schneider deleted the 313/pagination-button-position branch October 4, 2021 17:18
seanmalbert pushed a commit that referenced this pull request Jun 23, 2022
* Align pagination buttons with listings

* messing around with pulling styles into a class

* Rework the class using tailwind style rules

* Move "width: 100%" so that it always applies, regardless of screen size

* Pass styling options in as a AgPaginationProp (so that this change only affects one instance of AgPagination)

* Fix the border issues by passing in an includeBorder prop

* Switch max-width param to be a simple boolean (match the width of the listing card: yes or no?)

* Rename data-pager to data-pager-container

* Rename pagination-bar to data-pager

* Fix variable names

* Refactor the include-border and data-pager-container classes
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.

2 participants