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 design fixes #13590

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

Hardik500
Copy link
Contributor

@Hardik500 Hardik500 commented Oct 4, 2022

Description

Changed the design of pagination component to match more with the current theme

Dark Mode:
image

Light Mode:
image

Disabled Dark Mode:
image

Disabled Light Mode:
image

Hover Dark Mode:
image

Hover Light Mode:
image

Related Issue(s)

Fixes #12537, #12027

How to test

Go to any page which has some pagination involved and check the pagination buttons

Release Notes

Improve pagination usability

Documentation

Werft options:

  • /werft with-preview

@Hardik500 Hardik500 requested a review from a team October 4, 2022 17:11
@werft-gitpod-dev-com
Copy link

annotations in the pull request changed, but user is not allowed to start a job

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 5, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.0
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 5, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.1
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 5, 2022

/werft run with-preview with-payment

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.2
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 5, 2022

/werft run with-preview with-payment

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.3
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented Oct 12, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.4
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented Oct 13, 2022

@gtsiolis The preview env had to be run manually[internal]. Now the preview is available in case you'd like to test it.

@meysholdt
Copy link
Member

hi @Hardik500, For us to be able to merge this PR, can you please sign our CLA via this DocuSign form? If there are any questions, you can reach me via [email protected].

@Hardik500
Copy link
Contributor Author

@meysholdt I've signed the DocuSign

@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Oct 14, 2022
@meysholdt
Copy link
Member

Thank you for signing the CLA @Hardik500!

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 14, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.5
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hardik-pagination-design-fixes.1 because the annotations in the pull request description changed
(with .werft/ from main)

@vulkoingim
Copy link
Contributor

vulkoingim commented Oct 14, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.7
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented Oct 28, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.8
(with .werft/ from main)

@laushinka laushinka force-pushed the hardik/pagination-design-fixes branch from 6e50bc7 to 7717c70 Compare October 28, 2022 12:37
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hardik-pagination-design-fixes.2 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hardik-pagination-design-fixes.4 because the annotations in the pull request description changed
(with .werft/ from main)

@laushinka laushinka force-pushed the hardik/pagination-design-fixes branch from 1864607 to 490e4ed Compare October 28, 2022 13:45
@laushinka
Copy link
Contributor

Hi @Hardik500, sorry for the long delay in reviewing! I hope you don't mind that I rebased your branch so that we don't have to go back and forth too much again.
From testing I found this issue where the pagination disappeared on number 4.

I recorded a short loom. Let me know if you can't view it.

@Hardik500
Copy link
Contributor Author

Hi @Hardik500, sorry for the long delay in reviewing! I hope you don't mind that I rebased your branch so that we don't have to go back and forth too much again. From testing I found this issue where the pagination disappeared on number 4.

I recorded a short loom. Let me know if you can't view it.

Hey @laushinka , I am unable to reproduce it. Attaching the video ref below

Pagination.For.Workspace.mov

@laushinka
Copy link
Contributor

@Hardik500 Thanks for the recording! That does help. It could be my test cases in my preview environment that's causing it. Could you squash and rebase?

isDisabled={currentPage === 1}
onClick={prevPage}
label={"Previous"}
arrowReversed={false}
Copy link
Contributor

@laushinka laushinka Oct 31, 2022

Choose a reason for hiding this comment

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

Instead of calling this arrowReversed and accepting a boolean, could you keep it as arrowDirection and accepts a string? This prop is used not only for left and right, but also up and down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question, @laushinka so for "up" and "down" arrowDirection, do I need to handle code for those too in PaginationNavigationButtons component. Cause right now the code flow would be like this?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to handle "up" and "down" here because we won't see those in Pagination. The intention is to be consistent with what the <Arrow /> component expects :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, I have made the respective changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Could you squash and rebase please? :)

@Hardik500 Hardik500 requested a review from laushinka November 1, 2022 17:59
@laushinka laushinka requested a review from gtsiolis November 2, 2022 14:30
@gtsiolis gtsiolis force-pushed the hardik/pagination-design-fixes branch 2 times, most recently from c6d715a to 05244d8 Compare November 2, 2022 16:12
@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 2, 2022

/werft run with-preview

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.10
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hardik-pagination-design-fixes.8 because the annotations in the pull request description changed
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 2, 2022

Rebased and pushed, but job keeps failing. Looping this internally, see relevant discussion (internal). Cc @laushinka

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hardik-pagination-design-fixes.10 because the annotations in the pull request description changed
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 2, 2022

/werft run with-clean-slate-deployment with-preview

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.11
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented Nov 4, 2022

I'm going to approve this to unblock, but holding due to:

  • Checks not taking into account the already green build
  • @gtsiolis' review

/hold

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hardik-pagination-design-fixes.16 because the annotations in the pull request description changed
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2022

Looking at this now! 👀

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2022

/werft run with-clean-slate-deployment with-preview

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.12
(with .werft/ from main)

@gtsiolis gtsiolis force-pushed the hardik/pagination-design-fixes branch from 05244d8 to 12d59f4 Compare November 4, 2022 12:19
@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2022

/werft run with-clean-slate-deployment with-preview

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.13
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented Nov 4, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.14
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2022

/werft run

👍 started the job as gitpod-build-hardik-pagination-design-fixes-fork.15
(with .werft/ from main)

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks @Hardik500 @laushinka! 🔮

I couldn't test this sufficiently as the preview environment is getting removed always under ~15 minutes, see relevant discussion (internal). Cc @vulkoingim @mads-hartmann

From what I see from the screenshots, this looks good. We can always make minor adjustments later in the styles. ✔️

I hope this is not adding a pagination in the workspaces list as seen in #13590 (comment). From the code changes it seems not but let me know if this is not the case. ⚠️

Approving and merging.

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2022

/unhold

@roboquat roboquat merged commit 63deadd into gitpod-io:main Nov 4, 2022
@Hardik500
Copy link
Contributor Author

Hardik500 commented Nov 4, 2022

Thanks @Hardik500 @laushinka! 🔮

I couldn't test this sufficiently as the preview environment is getting removed always under ~15 minutes, see relevant discussion (internal). Cc @vulkoingim @mads-hartmann

From what I see from the screenshots, this looks good. We can always make minor adjustments later in the styles. ✔️

I hope this is not adding a pagination in the workspaces list as seen in #13590 (comment). From the code changes it seems not but let me know if this is not the case. ⚠️

Approving and merging.

Thanks @gtsiolis
Don't worry this won't be adding any pagination to workspaces. I just added some dummy code to test my changes easily

@laushinka
Copy link
Contributor

laushinka commented Nov 4, 2022

Thank you @Hardik500 for the patience and @gtsiolis for the co-reviewing 🙏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination links not properly themed
6 participants