-
Notifications
You must be signed in to change notification settings - Fork 7
Change to New Menu Structure #359
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
Conversation
✔️ Deploy Preview for cmu-delphi-main ready! 🔨 Explore the source changes: 8e93505 🔍 Inspect the deploy log: https://app.netlify.com/sites/cmu-delphi-main/deploys/6166223413aa0f0007fd79e3 😎 Browse the preview: https://deploy-preview-359--cmu-delphi-main.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is intended, since the top level menu item has two purposes like showing the sub menu but also linking to a page. As far as I remember we decided to duplicate the top menu link since it isn't obvious that you can also click on the top menu when the submenu is already popping up as a reaction to your mouse interaction. |
I'm collecting comments through Slack and will transfer them into change requests here |
hmmm, or i can just commit to your branch, up to you |
might be more efficient since it just changing the |
New nav LGTM - I agree that we should have the persistent nav label and 1st menu item link to the same page. Thanks guys. |
re breadcrumbs: the top level pages, e.g. /about didn't have breadcrumb so far, since it would just be an entry that is not clickable. that is also the reason why Blog (/blog) doenst have a submenu, since for compatibility reason it keeps the old url (/blog) which is top level. |
Wow thanks @sgratzl that was fast work! Two things remaining.
|
blog should now also have the proper breadcrumb. Pages without breadcrumbs are the www-covidcast related pages, since from a layout point of view it doesn't fit there atm.
I would suggest to come back to that after we integrated the new upcombing major changes to www-covidcast. |
|
Small thing I noticed in case it's possible to fix before merging. If not, then this is not critical. When you decrease the browser width, the menu item "COVID-19" gets split up over two lines. This is a bit awkward because I think it would be more natural to split, for example, "Flu & Other Pathogens". |
We're going to put this PR on hold until after October 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Ryan says okay
@sgratzl fix conflicts pls |
@krivard I resolved the conflicts. However, netlfy didn't bulid the latest update: https://app.netlify.com/sites/cmu-delphi-main/deploys. I don't why. Please check and test the website again before merging |
It seems to be showing that 8e93505 was indeed deployed; is that not what you see? |
Double-checking with @ryantibs that you wanted Flu & Other Pathogens to have nothing on it: |
Yes, for now. After the menu restructuring merge, there are two TODO items (@krivard, if you can, please create two issues and assign them to me and Roni, respectively):
|
No description provided.