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

feat(ui): remember pagination option, project search selection jumps into project #2821

Merged
merged 8 commits into from
Oct 29, 2024

Conversation

sergiofteixeira
Copy link

Hello, my first contribution.

This adds browser local storage to remember pagination option.

Also improves the search to show only what you searched for and when selection a project you jump into it.

@sergiofteixeira sergiofteixeira requested a review from a team as a code owner October 23, 2024 09:24
Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit ba2f752
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/6720a57a8b417a0008eccfee
😎 Deploy Preview https://deploy-preview-2821.docs.kargo.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sergiofteixeira sergiofteixeira changed the title feat:ui: remember pagination option, project search selection jumps into project feat(ui): remember pagination option, project search selection jumps into project Oct 23, 2024
@Marvin9 Marvin9 self-requested a review October 25, 2024 18:29
Copy link
Contributor

@Marvin9 Marvin9 left a comment

Choose a reason for hiding this comment

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

Couple of changes that needs to be in place before merging. Thanks for the PR!

ui/src/features/project/list/projects-list.tsx Outdated Show resolved Hide resolved
ui/src/features/project/list/projects-list.tsx Outdated Show resolved Hide resolved
@sergiofteixeira
Copy link
Author

sergiofteixeira commented Oct 28, 2024

Updated the pr with the changes requested @Marvin9

@Marvin9
Copy link
Contributor

Marvin9 commented Oct 28, 2024

There is just one bug indirectly blocking this PR that I am looking at ie. in local storage change the page size to the page which does not exist and refresh project list page

@sergiofteixeira
Copy link
Author

sergiofteixeira commented Oct 28, 2024

There is just one bug indirectly blocking this PR that I am looking at ie. in local storage change the page size to the page which does not exist and refresh project list page

useEffect(() => {
  if (data && page > Math.ceil(data.total / pageSize)) {
    setPage(Math.ceil(data.total / pageSize) || 1);
  }
}, [data, page, pageSize, setPage]);

@Marvin9 maybe use useEffect to check if data is available? else reset it. just an idea

so when you set the page number on ur storage to higher than the number of pages, it goes back to first, want me to add it to this PR? ( tested it locally and works )

@Marvin9
Copy link
Contributor

Marvin9 commented Oct 28, 2024

@sergiofteixeira yes please push that change. I tested in local and it resolves the issue that I mentioned

@Marvin9
Copy link
Contributor

Marvin9 commented Oct 28, 2024

Also don't forget to run make lint-ui

Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
…ger fix

Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
@sergiofteixeira
Copy link
Author

@Marvin9 pushed those changes and ran lint

Copy link
Contributor

@Marvin9 Marvin9 left a comment

Choose a reason for hiding this comment

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

One last change and then we can merge after CI checks. Thanks!

Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
@sergiofteixeira
Copy link
Author

pushed the change @Marvin9

Signed-off-by: Sergio Teixeira <sergio@triggerise.org>
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.01%. Comparing base (7490ff2) to head (ba2f752).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2821      +/-   ##
==========================================
+ Coverage   48.82%   50.01%   +1.18%     
==========================================
  Files         270      274       +4     
  Lines       23941    24386     +445     
==========================================
+ Hits        11690    12197     +507     
+ Misses      11620    11539      -81     
- Partials      631      650      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Marvin9 Marvin9 added this pull request to the merge queue Oct 29, 2024
Merged via the queue into akuity:main with commit fedf4aa Oct 29, 2024
17 checks passed
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.

3 participants