Skip to content

Make right-side menus onclick. #1096

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

Closed
wants to merge 4 commits into from
Closed

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 14, 2020

This is followup from #1081 and #1077.

This takes the link that was the click action for each menu, and moves it to the first item in the menu.

@Nemo157
Copy link
Member

Nemo157 commented Oct 14, 2020

One issue here is that the pure-menu-opt-children class suppresses showing the children on mobile, turning the "about" menu item into just a direct link to the main about page; I'm not sure how that will behave now that it doesn't link.

@jsha
Copy link
Contributor Author

jsha commented Oct 14, 2020

Ah, good catch. I tested this using the mobile simulator in Chrome, and tapping the "(i)" icon (which is what the "about" menu becomes) does nothing.

So, this change is not ready to merge yet. I'm not sure how to make the item act as a link on mobile (or small screens), while acting to open the menu on larger screens. I'm open to ideas!

@jyn514 jyn514 added A-frontend Area: Web frontend S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Oct 14, 2020
@jyn514 jyn514 marked this pull request as draft October 14, 2020 23:59
@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

Unfortunately I don't know enough about CSS to help you here, but @GuillaumeGomez might have ideas.

<li class="pure-menu-item pure-menu-has-children pure-menu-allow-hover pure-menu-opt-children">
<a href="/about" class="pure-menu-link">
<li class="pure-menu-item pure-menu-has-children pure-menu-opt-children">
<a href="#" class="pure-menu-link">
Copy link
Member

Choose a reason for hiding this comment

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

Why keeping this element then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I removed the <a> tag and just had the text "About", clicks on "About" didn't do anything.

#}<li class="pure-menu-item pure-menu-has-children pure-menu-allow-hover pure-menu-opt-children">
<a href="/releases" class="pure-menu-link">
#}<li class="pure-menu-item pure-menu-has-children pure-menu-opt-children">
<a href="#" class="pure-menu-link">
Copy link
Member

Choose a reason for hiding this comment

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

Why keeping this element?

@jsha
Copy link
Contributor Author

jsha commented Oct 15, 2020

I though of a solution to the mobile problem: Right now menu.js has a handler that ignores clicks on # links. I could change that to also ignore clicks on links that are part of pure-menu-opt-children, unless the children are hidden due to being on a small screen, in which case it would allow those clicks through.

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

I though of a solution to the mobile problem: Right now menu.js has a handler that ignores clicks on # links. I could change that to also ignore clicks on links that are part of pure-menu-opt-children, unless the children are hidden due to being on a small screen, in which case it would allow those clicks through.

Sounds like a plan :)

@jsha
Copy link
Contributor Author

jsha commented Dec 30, 2021

I'm going to close this for now; it has conflicts and I'm not likely to pick it up again soon.

@jsha jsha closed this Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Web frontend S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants