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

fix: breadcrumb navigation css #4

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

navinkarkera
Copy link
Member

The breadcrumb navigation popup was hidden due to css property overflow: hidden in parent li element. This is a tricky issue in css where we want the parent to have overflow:hidden normally (to hide content overflowing the container) but show popups from its children element.

It was working normally in nutmeg version as the parent element of popup (which has position: absolute) did not have position: relative, causing the popup to be displayed in the top left of the breadcrumb parent div like so:

image

This was fixed in palm by this commit but it now positioned the popup inside list element which has overflow:hidden property.

We are overriding the overflow property on focus to retain both behaviour.

image

Test instructions:

  • Setup latest tutor (palm) locally
  • Checkout this PR/branch.
  • Mount frontend-app-learning using tutor config save --append MOUNTS=/path/to/frontend-app-learning
  • Run tutor dev launch
  • Visit any course and use the breadcrumb navigation buttons. Note: You need to login using staff/admin user as breadcrumbs with navigation feature is only visible to staff users.

@navinkarkera navinkarkera self-assigned this Nov 6, 2023
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: tested the course navigation
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@Agrendalath Agrendalath merged commit 0137868 into opencraft-release/palm.1 Nov 7, 2023
2 of 3 checks passed
@Agrendalath Agrendalath deleted the navin/palm/fix-breadcrumbs branch November 7, 2023 19:05
@0x29a
Copy link
Member

0x29a commented Apr 11, 2024

@navinkarkera, do we need to upstream this PR? Also, what ticket it relates to (if any)?

Edit: nevermind, I see the note in the code drift project.

@Cup0fCoffee
Copy link
Member

@navinkarkera @Agrendalath I'm assuming this PR is not needed in Redwood or Sumac, so going to remove it from the code drift dashboard, but if it's needed, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants