-
Notifications
You must be signed in to change notification settings - Fork 441
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
Refactored community & collection pages #2722
Refactored community & collection pages #2722
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.
@alexandrevryghem : Thanks for this work! I've verified all the accessibility bugs linked in the description are fixed. I've tested all the Community/Collection features that I could think of (including everything related to browse options & all editing options too). Everything seems to work well overall, but I've found what looks to be a very minor bug with the "Browse by Issue Date" year field.
Here's what I'm doing:
- First, browse to a Community or Collection
- Click on Browse "By Issue Date" tab.
- Immediately click on the "Choose year" dropdown. It will be empty. (This is the bug)
If you browse to a different Browse option on that same Community or Collection and return to the "By Issue Date" tab, then the year field will show all it's options. So, the bug appears to be that the menu isn't loaded the first time you access the page.
NOTE: This bug also appears in the global Browse by Issue Date. It is not reproducible on https://sandbox.dspace.org, so I'm guessing it may be caused by this PR.
Beyond that, this PR looks great. I've also done a code review and I don't notice anything concerning.
@tdonohue: Thnx for the review! I fixed the issue, it was caused because these values in the injector were only initialised during the |
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 @alexandrevryghem ! I can confirm this works perfectly now. The code looks good too.
This PR is waiting on #2339 (which it depends on) to be merged. @artlowel : if you can find a moment in the next few days, it'd be good to add your feedback to #2339...as this PR further builds in that same direction.
Once these two PRs are ready, I'm currently planning to try to port these to 7.6.x unless anyone has objections. I figured it'd be nice to ensure the accessibility fixes and general code cleanup get ported back.
@tdonohue: If PR #2339 is going to get back ported I can also add these changes to that PR. On that branch I added improvements that I thought should be back ported to 7.6.2:
|
Hi @alexandrevryghem, |
43ea927
to
2a957eb
Compare
Removed UPDATE: See #2800 for the partial port of this PR |
…ubCollectionListComponent to a custom sections folder
…ity/collection pages
- The data passed to the injector in BrowseByComponent was not updated by ngOnChanges - Also refactored the injector logic to StartsWithLoaderComponent
@alexandrevryghem : If you can get this updated (all merge conflicts are caused by the merger of #2339), I'd love to get this re-reviewed quickly as other work depends on it. |
…col-pages_contribute-main
2a957eb
to
193d56d
Compare
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.
👍 Thank you @alexandrevryghem ! I retested this today, and it's working well. Verified it fixes the two linked issues and that no new issues have appeared in this refactor.
I'll next test that the (partial) port of this to dspace-7_x
in #2800 is working for that branch.
References
Description
The reason why the community/collection title remained was because the global browse by page and the community & collection browse pages used the same component (
BrowseByMetadataPageComponent
). When you went to a community/collection browse page the comcol header & footer that were hardcoded on the global browse page were shown, otherwise they were hidden. Because changing from scope didn't change the component that should be rendered (because both wit or without scope the global browse page was displayed), the page didn't refresh completely. This isn't really a clean way of creating tabs because the community & collection header & footer were duplicate. The cleanest way was to completely refactor the global browse pages and to extract the code that needed to be displayed on both the comcol pages and the global browse pages and move them into browse sections. These sections can then be displayed on the newly createdBrowseByPageComponent
or as a tab on the community/collection pages.Instructions for Reviewers
List of changes in this PR:
SubComColSectionComponent
containing the community'sSubcommunities and Collections
tab contentCollectionRecentlyAddedComponent
containing collection's theRecent Submissions
tab contentBrowseByMetadataPageComponent
because now this is not needed anymoreapp-routlet
ComcolBrowseByComponent
that will display the browse sections for both community & collection pagesBrowseByMetadataPageComponent
that will be used to render the global browse by pages by using the browse sectionsDuring this PR I also found & fixed additional bugs:
dateissued
has an infinite load animation when there are 0 items to displayloading$
variable to that is automatically upated when the backend sends a response backbrowse
linkComcolBrowseByComponent
the rendered browse section weren't automatically updated. This was because the parameters (in this casebrowseByType
) who are used to retrieve the component ingetComponent()
changed without calling thegetComponent
method again. Because the component wasn't reinitialised switching between 2 browse tabs didn't have any effect and it looked like you were still on the same page.@rendersBrowseBy(BrowseByDataType, theme)
Guidance for how to test or review this PR:
dateissued
tab correctly displays the messageNo items to show.
instead of the infinite load animationChecklist
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.