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

Refactored community & collection pages #2722

Merged

Conversation

alexandrevryghem
Copy link
Member

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 created BrowseByPageComponent or as a tab on the community/collection pages.

Instructions for Reviewers

List of changes in this PR:

  • Created the SubComColSectionComponent containing the community's Subcommunities and Collections tab content
  • Created the CollectionRecentlyAddedComponent containing collection's the Recent Submissions tab content
  • Removed the comcol header & footer from the BrowseByMetadataPageComponent because now this is not needed anymore
  • Updated the community & collection routing modules in order to be able to work with the app-routlet
  • Created a new tab ComcolBrowseByComponent that will display the browse sections for both community & collection pages
  • Created a new page BrowseByMetadataPageComponent that will be used to render the global browse by pages by using the browse sections

During this PR I also found & fixed additional bugs:

  • The browse by dateissued has an infinite load animation when there are 0 items to display
    • Fixed this by creating a separate loading$ variable to that is automatically upated when the backend sends a response back
  • The hierarchical browse pages didn't support scoped searches
    • Fixed this by adding the scope to the search query when clicking on the browse link
  • When you switch between 2 tabs who used the ComcolBrowseByComponent the rendered browse section weren't automatically updated. This was because the parameters (in this case browseByType) who are used to retrieve the component in getComponent() changed without calling the getComponent 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.
    • Because this is a general component loader bug I moved the fix to Created abstract component loader class #2339 (instead of fixing it in all the loaders individually). This is also the reason why this PR is dependant from that PR
  • Themed versions of the browse pages were created, but these are dynamically loaded components using custom decorators, so the correct way to theme those is using the theme parameter in @rendersBrowseBy(BrowseByDataType, theme)

Guidance for how to test or review this PR:

  • Verify that the community & collection tabs work
  • Verify that the browse sections on the community & collection pages don't display items from different collections
  • Verify that the community & collection browse by dateissued tab correctly displays the message No items to show. instead of the infinite load animation
  • Verify that the accessibility issues on the browse page are fixed
  • Verify that the hierarchal browse tabs on the community & collection pages now also display their respective header & footer and that the browse button only displays results inside that scope
  • Verify that the global hierarchal browse doesn't set a scope

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@alexandrevryghem alexandrevryghem self-assigned this Dec 20, 2023
@alexandrevryghem alexandrevryghem added bug accessibility component: Discovery related to discovery search or browse system performance / caching Related to performance, caching or embedded objects medium priority component: Community Community display or editing component: Collection Collection display or editing claimed: Atmire Atmire team is working on this issue & will contribute back labels Dec 20, 2023
@tdonohue tdonohue self-requested a review January 8, 2024 16:25
Copy link
Member

@tdonohue tdonohue left a 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)
    empty-year-browse-date

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 tdonohue added the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Jan 26, 2024
@alexandrevryghem
Copy link
Member Author

@tdonohue: Thnx for the review! I fixed the issue, it was caused because these values in the injector were only initialised during the ngOnInit instead of updating them each time they changed (it's weird that it never came up until now 😅). Moving that code to the ngOnChanges method fixed the issue. Afterwards I also refactored the injector logic into the new StartsWithLoaderComponent. This way that component doesn't need to be regenerated every time the ngOnChanges is triggered.

Copy link
Member

@tdonohue tdonohue left a 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 tdonohue added this to the 8.0 milestone Jan 29, 2024
@alexandrevryghem
Copy link
Member Author

@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:

  • I replaced all the remaining usages of the deprecated ComponentFactoryResolver
  • I ensured that every dynamically generated componentRef is destroyed in ngOnDestroy() by calling the destroy() on the componentRef
  • I also added an if statement around the instantiateComponent() call in ngOnInit in order to prevent the component from being recreated a second time (first it is created by ngOnChanges and then it is recreated a second time when the ngOnInit is triggered).

Copy link

Hi @alexandrevryghem,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@alexandrevryghem alexandrevryghem force-pushed the refactor-comcol-pages_contribute-main branch from 43ea927 to 2a957eb Compare February 2, 2024 11:04
@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Feb 6, 2024
@tdonohue
Copy link
Member

tdonohue commented Feb 6, 2024

Removed port to dspace-7_x label as this will need to be ported manually. Discussed this with @alexandrevryghem and he noted that the only fix in this PR which should be ported is 6adac10

UPDATE: See #2800 for the partial port of this PR

…ubCollectionListComponent to a custom sections folder
@tdonohue
Copy link
Member

tdonohue commented Feb 9, 2024

@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.

@alexandrevryghem alexandrevryghem force-pushed the refactor-comcol-pages_contribute-main branch from 2a957eb to 193d56d Compare February 9, 2024 22:55
Copy link
Member

@tdonohue tdonohue 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 @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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility bug claimed: Atmire Atmire team is working on this issue & will contribute back component: Collection Collection display or editing component: Community Community display or editing component: Discovery related to discovery search or browse system high priority performance / caching Related to performance, caching or embedded objects
Projects
No open projects
Status: ✅ Done
2 participants