Skip to content

Conversation

@alexandrevryghem
Copy link
Member

References

Description

When a subsection is added to a menu section that has no subsections, the subsections won't appear if the component has already been rendered. To fix this we now not only check if new sections were added/removed, but also if the component that should be used to render that specific section has changed. This way the components will render again when we add a subsection to a section, because then the top level id still won't have changed, but the component that the top level sections should use to render will have.

Instructions for Reviewers

List of changes in this PR:

  • The distinctUntilChanged(compareArraysUsingIds) check from MenuComponent has been moved to MenuService#getMenuTopSections
  • In MenuComponent the distinctUntilChanged that is responsible for setting the value of sectionMap$ now also checks it the component that should be renderd has changed.

Guidance for how to test/review this PR:

  1. Add the following code to your HomeNewsComponent:
ngOnInit(): void {
  this.menuService.addSection(MenuID.PUBLIC, Object.assign({
    id: 'test',
    parentID: 'browse_global_communities_and_collections',
    active: false,
    visible: true,
    model: {
      type: MenuItemType.LINK,
      text: 'test',
      link: '/test',
    } as LinkMenuItemModel,
  }, {
    shouldPersistOnRouteChange: true,
  }));
}
  1. Go to a random page (except for home, for example /statistics) and refresh the page
  2. The Communities & Collections should have no subsection yet
  3. Navigate to the home page using the breadcrumbs
  4. The subsection is now visible underneath Communities & Collections

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 Nov 9, 2023
@alexandrevryghem alexandrevryghem added bug claimed: Atmire Atmire team is working on this issue & will contribute back labels Nov 9, 2023
@tdonohue tdonohue self-requested a review November 9, 2023 17:15
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 was able to verify that the current behavior does not update to load the new subsection. Also verified that this PR fixes the issue.

In case anyone else is looking at this in the future, here's how I tested it:

  1. Updated HomeNewComponent with this exact code:
import { OnInit } from '@angular/core';
import { Component, } from '@angular/core';
import { MenuID } from 'src/app/shared/menu/menu-id.model';
import { MenuItemType } from 'src/app/shared/menu/menu-item-type.model';
import { LinkMenuItemModel } from 'src/app/shared/menu/menu-item/models/link.model';
import { MenuService } from 'src/app/shared/menu/menu.service';

@Component({
  selector: 'ds-home-news',
  styleUrls: ['./home-news.component.scss'],
  templateUrl: './home-news.component.html'
})

/**
 * Component to render the news section on the home page
 */
export class HomeNewsComponent implements OnInit {

  public constructor(
    protected menuService: MenuService,
  ) {
  }


  ngOnInit(): void {
    this.menuService.addSection(MenuID.PUBLIC, Object.assign({
      id: 'test',
      parentID: 'browse_global_communities_and_collections',
      active: false,
      visible: true,
      model: {
        type: MenuItemType.LINK,
        text: 'test',
        link: '/test',
      } as LinkMenuItemModel,
    }, {
      shouldPersistOnRouteChange: true,
    }));
  }
}
  1. Redeployed in prod mode (yarn build;prod; yarn serve:ssr)
  2. Visited the Community list page first: http://localhost:4000/community-list/
    • No "test" menu under "Communities & Collections" should exist as the HomeNewsComponent has not loaded.
  3. Then clicked on "Home" in breadcrumbs.
    • Without this PR, no "test" menu continues to exist
    • With this PR, the "test" menu now appears under "Communities & Collections

@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 Nov 13, 2023
@tdonohue tdonohue merged commit cb70190 into DSpace:main Nov 13, 2023
@dspace-bot
Copy link
Contributor

Backport failed for dspace-7_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin dspace-7_x
git worktree add -d .worktree/backport-2610-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-2610-to-dspace-7_x
git checkout -b backport-2610-to-dspace-7_x
ancref=$(git merge-base 7cf26e48150a04a12a2c2ac4dcad28dbbc919333 1bbc053c0038de7ca37c43e92af6bf6fb3234b78)
git cherry-pick -x $ancref..1bbc053c0038de7ca37c43e92af6bf6fb3234b78

@tdonohue
Copy link
Member

Manual port to dspace-7_x in #2639

@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 Nov 13, 2023
@tdonohue tdonohue added this to the 8.0 milestone Nov 13, 2023
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Dec 17, 2024
Task/dspace cris 2023 02 x/DSC-2070

Approved-by: Andrea Barbasso
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug claimed: Atmire Atmire team is working on this issue & will contribute back

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

The menu doesn't update if a new sub section is added after rendering has already completed

4 participants