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

Mega Menus #1191

Merged
merged 24 commits into from
Aug 19, 2024
Merged

Mega Menus #1191

merged 24 commits into from
Aug 19, 2024

Conversation

jacob-korf
Copy link
Contributor

@jacob-korf jacob-korf commented Aug 9, 2024

Closes #629.
Adds a mega menu that can be attached as a block in a menu link.
Entities -> CuBoulder/tiamat-custom-entities#157
Profile - > CuBoulder/tiamat10-profile#180
Template -> CuBoulder/tiamat10-project-template#51

@jcsparks jcsparks changed the title Issue/629 Mega Menus Aug 12, 2024
@timurtripp timurtripp self-requested a review August 12, 2024 18:33
Copy link
Member

@timurtripp timurtripp left a comment

Choose a reason for hiding this comment

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

I'm getting a JavaScript error and broken display of the menu on the mobile view (entire main navigation is missing)

@timurtripp
Copy link
Member

Columns should be equal width and may need some margin/padding fixes
Screenshot 2024-08-12 at 12 37 53 PM

@timurtripp
Copy link
Member

Revert the changes in region--primary-menu.html.twig and try to keep code spacing / indentation as consistent as possible throughout.

@timurtripp
Copy link
Member

timurtripp commented Aug 12, 2024

Clicking the menu item results in a # being appended to the page URL. The linked URL of the menu item should not change after being made a Mega Menu, but with a preventDefault() call stopping the browser from following the link on click.

@timurtripp
Copy link
Member

timurtripp commented Aug 12, 2024

Using the following malicious menu link title:

Highhelm Clans19');alert('successfully executed');void('

I'm able to execute arbitrary JavaScript. This is an XSS vulnerability. Please properly sanitize all user input before passing it into scripts.

@timurtripp timurtripp added the new New feature or request label Aug 12, 2024
@timurtripp
Copy link
Member

On D7, clicking anywhere outside the mega menu hides it. That doesn't seem to work in this version.

@timurtripp
Copy link
Member

Text color inside the mobile menu on my site is white-on-white and not legible.

@timurtripp
Copy link
Member

Link href is still incorrectly altered by enabling mega menu – this time the attribute is gone completely.

@timurtripp
Copy link
Member

The HTML title attribute you added isn't present on D7, isn't needed for accessibility, and should be removed.

@jcsparks jcsparks merged commit 08d37c0 into main Aug 19, 2024
5 checks passed
@jcsparks jcsparks deleted the issue/629 branch August 19, 2024 17:23
github-actions bot pushed a commit that referenced this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mega Menu
3 participants