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

added condition to include nav item only if url is not empty #1449

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

umar8hassan
Copy link
Contributor

@umar8hassan umar8hassan commented Sep 24, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5358

Description (What does it do?)

Menu items can be links to pages or external resource links. On rendering resources are pulled from DB. If the linked resource has been deleted, the menu item still renders and is broken. On click, it does not show any error but doesn't navigate anywhere either.

This PR aims to conditionally render menu items if it has a valid URL field. If deleted the related field is empty.

How can this be tested?

  1. Create a test course on RC, add an External resource to its Menu, and Publish.
  2. In your local themes repo, switch to branch main
  3. Run command yarn start course <course-name-on-rc>, this should pull course from RC
  4. Browse the pulled course on localhost:3000 and validate the menu item is added and navigable
  5. After validation, go to the admin panel and delete the related External resource without removing it from menu
  6. Publish the course. Remove the local copy of course and repeat steps 3 and 4. The nav item will be there but not navigable.
  7. In your local themes repo, switch to branch umar/5358-allow-studio-user-to-delete-pages. The broken nav item will disappear.

@github-actions github-actions bot temporarily deployed to pull request September 24, 2024 12:56 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 24, 2024 12:56 Inactive
Copy link

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdpinch
Copy link
Member

pdpinch commented Sep 24, 2024

How/when does this happen?

@umar8hassan umar8hassan self-assigned this Sep 24, 2024
@umar8hassan
Copy link
Contributor Author

How/when does this happen?

@pdpinch It happens when the resource has been deleted in the backend but its entry to menu items is still there. I have added the description. this is part of deleting pages PR.

@umar8hassan umar8hassan marked this pull request as ready for review October 28, 2024 12:01
@ibrahimjaved12 ibrahimjaved12 self-assigned this Oct 29, 2024
Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works fine normally. However, there are 2 corner case side effects:

  1. Nested Menu has Menu Item (Child) which does not have link
  2. Nested Menu Parent does not have link, but has children

In the first case, the arrow toggles but opens nothing:
image

In the second case, we have an empty parent item with an arrow:
image

Also, in line 28 we have:
{{ if .menuItem.HasChildren }}
We should replace it with {{ if $hasChildren }} as we already have this variable for consistency.

@github-actions github-actions bot temporarily deployed to pull request October 30, 2024 12:19 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 30, 2024 12:19 Inactive
Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue 1: Broken nested parents are marked as active by default

Under current code, for broken nested parents, we end up having empty anchor tags, eg.
<a>A menu item but without href that will be displayed</a>
The active status is determined by this condition in base-theme/assets/js/navigation.js:
navLink.replace(/\/$/, "") === window.location.pathname.replace(/\/$/, "")
If href is absent, navLink becomes an empty string (""). When the current path (window.location.pathname) is the homepage, this empty string falsely matches, leading to a false-positive and incorrectly adding the active class.
eg.:
<a class="text-dark nav-link active" data-uuid="26c26fef-7262-4f7f-960f-20e462964b18">medium ER</a>

image

Issue 2: Mobile interface is being affected by broken links
<li class="course-nav-list-item"></li> elements still appear to be appended for broken menu items, causing layout issues.

PR View:
image

Same items in RC:
image

Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality works as expected.
But can you please add the error message prior to clicking Delete button on Dialog.

Sorry this comment was meant for another PR. Please follow the above comment only.

@github-actions github-actions bot temporarily deployed to pull request November 6, 2024 00:41 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 6, 2024 00:41 Inactive
Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@umar8hassan umar8hassan merged commit 71b4557 into main Nov 11, 2024
7 of 8 checks passed
This was referenced Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants