-
Notifications
You must be signed in to change notification settings - Fork 439
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
Fix for User profile (/profile): only 20 group memberships shown instead of all #3105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorHugoDuranS : Thanks! We also reviewed this PR together as a team in our DSpace Developer Meeting. The feedback here is similar. We need to have a PR created against main
so that we can apply these changes to 8.x and 9.0. Overall, though, the code looks reasonable (except for the one note by @artlowel above).
We didn't get a chance to test this, but I'll volunteer to help test it (if no one else gets to it) once we have a version of this PR against the main
branch
Hi @VictorHugoDuranS, |
# Conflicts: # src/app/profile-page/profile-page.component.ts # src/assets/i18n/ar.json5 # src/themes/custom/lazy-theme.module.ts
@tdonohue I merged the main branch to this branch with the current changes. Thanks for the feedback!. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorHugoDuranS : Apologies for the delay in reviewing this again. I tested this today, and the basic functionality works, but I'm seeing a number of errors in my brower's DevTools anytime I visit the /profiles
page. These seem to be caused by the new code.
Here's what I'm doing:
- Install this PR and run in production mode (
npm install; npm run build:prod; npm run serve:ssr
) - Open Chrome DevTools and visit the site.
- Login and visit the
/profiles
page. In my Chrome DevTools "Console", I see these errors logged:Error TypeError: Cannot read properties of null (reading 'isLoading')
Error TypeError: Cannot read properties of undefined (reading 'createComponent')
- Neither of the above errors occur on https://sandbox.dspace.org, or if I run the frontend without this PR installed. That implies these errors are based on the code in this PR.
Beyond that, the pagination is working for me. So, this can be merged once the errors are fixed.
(As a sidenote, you do not need to modify every json5 file in the future, when you add a new translation key. It's only necessary to modify the "en.json5" file, and the others will be updated the next time they are retranslated.)
Hi @VictorHugoDuranS, |
Hi @tdonohue , I made the corrections to address the remarks you mentioned in a previous comment, I also updated the translations to leave only English. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorHugoDuranS and @VictorDuranEscire : This is looking better. The bugs are fixed. I tested it today and no longer see any errors. However, this PR is still modifying *every single .json5 file by adding in unnecessary blank lines. Please remove those unnecessary changes from this PR, as they can make the PR much more difficult to backport to 8.x and 7.x.
Ideally, your PRs should be as small as possible without making any unnecessary formatting changes, etc. Thanks!
Hi @tdonohue , thanks for the feedback, I made the adjustments to remove the changes to the .json5 files only leaving the English translation. I will consider these comments in my future PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @VictorHugoDuranS ! This now looks good to me. Merging and porting to 8.x & 7.x
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-7_x
git worktree add -d .worktree/backport-3105-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-3105-to-dspace-7_x
git switch --create backport-3105-to-dspace-7_x
git cherry-pick -x 7ab4af864302373edd51d74987253a55f23c6667 65fb412fd19af3f867bceca74f4df90160f40368 513af21cd45748a72cf5b17b2527cfdae191b9a0 1d67a9cfae3357af64839a75fdd862d0ced0bf70 67f72b77bd651a29d95d5c544e5deaf24a75e7e0 00a5fd8f6a048b16af1ed7599acb5981d19d42e3 19552598fcafcd00b280622543e495a7b0caedaf ce33d3a04f6352856b6556887f3064581b2434cc 6156a40db76618e14b3da2ec161b992413b41851 eced5856ab98999c396e21a4aff1622833bde53a 1872b6224c2c8cdd6aa549fc4ad417d126f78c09 |
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-8_x
git worktree add -d .worktree/backport-3105-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-3105-to-dspace-8_x
git switch --create backport-3105-to-dspace-8_x
git cherry-pick -x 7ab4af864302373edd51d74987253a55f23c6667 65fb412fd19af3f867bceca74f4df90160f40368 513af21cd45748a72cf5b17b2527cfdae191b9a0 1d67a9cfae3357af64839a75fdd862d0ced0bf70 67f72b77bd651a29d95d5c544e5deaf24a75e7e0 00a5fd8f6a048b16af1ed7599acb5981d19d42e3 19552598fcafcd00b280622543e495a7b0caedaf ce33d3a04f6352856b6556887f3064581b2434cc 6156a40db76618e14b3da2ec161b992413b41851 eced5856ab98999c396e21a4aff1622833bde53a 1872b6224c2c8cdd6aa549fc4ad417d126f78c09 |
…ead of all (DSpace#3105) * Add - groups paginated on profile page * Add - groups paginated on profile page * Add - groups paginated on profile page * Add - groups paginated on profile page * Fix UPDATE - Add pagination message error and loader * Update BRANCH * Fix - LINT ERRORS * Fix: Error declaring variable for group pagination * Fix: Remove unnecessary translations for paging groups * Fix: Lint erros * Fix: Remove unnecessary translations --------- Co-authored-by: VictorDuranEscire <victor@escire.lat>
…ead of all (DSpace#3105) * Add - groups paginated on profile page * Add - groups paginated on profile page * Add - groups paginated on profile page * Add - groups paginated on profile page * Fix UPDATE - Add pagination message error and loader * Update BRANCH * Fix - LINT ERRORS * Fix: Error declaring variable for group pagination * Fix: Remove unnecessary translations for paging groups * Fix: Lint erros * Fix: Remove unnecessary translations --------- Co-authored-by: VictorDuranEscire <victor@escire.lat>
Hi @tdonohue , I'm @jtimal partner, I like to share this PR with you:
References
Description
Set pagination on groups of the user on profile page
Instructions for Reviewers
On the profile page I added the pagination of groups adding the pagination template and modifying the current service of groups for paginated it. I set 20 elements par page as default in the pagination
List of changes in this PR:
Example:
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.