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

Fixed home page issues #2844

Merged
merged 6 commits into from
Apr 12, 2024
Merged

Conversation

alexandrevryghem
Copy link
Member

@alexandrevryghem alexandrevryghem commented Feb 29, 2024

References

Description

Some fixes for issues on the home page related to new features merged in the past few weeks:

#2275:

  • The home page content (communities in dspace & recent submissions) is not aligned anymore with the home news content
  • For screens smaller than 576 pixels the home page facets are being displayed on top of the communities section & recent submission section
  • The home page used the non themed version of ds-themed-configuration-search-page
  • Hide the Reset filters button on the home page facets

#2681:

  • Fix minor memory leak on home page & footer (subscriptions not being cleaned up)
  • Created the missing route for /info/coar-notify-support and created an option to hide that link in the footer like done for EULA & Privacy Policy

Other minor fixes:

  • Fixed blank page being displayed instead of 404 when disabling EULA/Privacy Policy page and going to (/info/end-user-agreement//info/privacy)
  • Prevent /server/api/eperson/profiles/{userId} to be called when unauthenticated leading into a /server/api/eperson/profiles/undefined call
  • Fixed a minor alignment issue on /community-list where the names would not be vertically centred in comparison with the chevron icon and item count badge

Instructions for Reviewers

List of changes in this PR:

  • Rewrote the way the search facets are being rendered next to the home page content. I used the PageWithSidebarComponent to render them, this has some benefits for example on screen with a small height the facets bar will move along with you and won't stay at the top on long pages. It will also automatically use the whole width to display the search filters when your screen is between 576px & 768px
  • Display the expand collapse button of search facets on screens between 576px & 768px above the search bar
  • Moved COAR logic out of HomePageComponent and into a separate component

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 Feb 29, 2024
@alexandrevryghem
Copy link
Member Author

I thought about 2 additional minor improvements that I can add to this PR if there are no objections:

  • We could maybe hide the Reset filters button when inPlaceSearch is false, because it simply redirect you to the search page, and you can't select any filter on the home page (you are automatically redirected to the search page).
  • We could also maybe move the COAR code from the home page to a dedicated component. We can still call that component in the HTML of HomePageComponent, but this way we clearly seperate that functionality from the home page code

@tdonohue tdonohue added improvement 1 APPROVAL pull request only requires a single approval to merge labels Mar 1, 2024
@tdonohue tdonohue added this to the 8.0 milestone Mar 1, 2024
@tdonohue tdonohue self-requested a review March 1, 2024 14:23
Copy link

github-actions bot commented Mar 4, 2024

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

Copy link

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

@tdonohue
Copy link
Member

@alexandrevryghem : Would you have time to quickly rebase this on the latest main? I want to make sure this is compatible with the recent updates to Angular 16 (I suspect it will be, but currently it's not running on the latest code in tests)

@alexandrevryghem
Copy link
Member Author

@tdonohue: I retested it locally after merging the latest main and fixed a remaining spacing issue on the home page and also already implemented these suggestions

@tdonohue
Copy link
Member

@alexandrevryghem : it appears this PR accidentally includes a ton of unrelated commits now (it shows over 8,000 lines of code changed). Could you try to clean it up quickly so that I can test it today/tomorrow?

@alexandrevryghem
Copy link
Member Author

@tdonohue: Sry I don't know what happend 😬 I cherry-picked the changes back on the latest version of main

@tdonohue
Copy link
Member

@alexandrevryghem : No worries! I've been in the same situation... sometimes Git/GitHub does things that are unexplainable. It looks good now & I'll try to review this one tomorrow. Thanks!

Also fixed error that showed a blank page instead of a 404 when disabling EULA/Privacy Policy pages
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 ! This looks great! I also like the new configuration flag to optionally disable the COAR Notify support page, even if you have COAR Notify enabled.

I tested all the fixes locally and everything seems to be working

@tdonohue tdonohue merged commit c13d23d into DSpace:main Apr 12, 2024
13 checks passed
@alexandrevryghem alexandrevryghem deleted the fix-home-page-issues branch April 17, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge improvement
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants