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

Navigation Redesign #20

Merged
merged 7 commits into from
Feb 18, 2025
Merged

Navigation Redesign #20

merged 7 commits into from
Feb 18, 2025

Conversation

Toby-Rea
Copy link

Redesigned the navigation. The hamburger menu is now only displayed for mobile users, and should be much easier to work with

@Toby-Rea
Copy link
Author

Desktop:

image

Mobile:

image
image

@Toby-Rea Toby-Rea marked this pull request as ready for review January 31, 2025 00:12
@ittipatken
Copy link

Could you add a link to AnkiWeb? I think it is one of the most important links.

@Toby-Rea
Copy link
Author

Toby-Rea commented Feb 1, 2025

Could you add a link to AnkiWeb? I think it is one of the most important links.

That's a good point. On reflection I might refactor this. It may make more sense to move the in-page links into a floating table of contents on desktop, but keep them in the mobile menu.

@dae
Copy link
Member

dae commented Feb 2, 2025

Does that mean you'd prefer me not to merge this until you've refactored it? Or do you plan to make those changes in a follow-up PR?

@Toby-Rea
Copy link
Author

Toby-Rea commented Feb 2, 2025

I won't be able to revisit this for another couple days, but I'm happy for this to be merged in its current state. I'll try to keep things in focused PRs so I'll handle the table of contents separately. Thank you for checking in 😃

@dae
Copy link
Member

dae commented Feb 2, 2025

Sorry, I should have reviewed this prior to asking you that question :-) I have a couple of thoughts/concerns. I'd appreciate feedback from others like @ittipatken as well if they have anything to add.

  • When testing locally, I see a white top nav bar. Was that a deliberate change from your screenshot, or a regression caused by some other PR?
  • I seem to recall begrudgingly agreeing to include an add-ons link, only because it was buried in a hamburger menu and not too prominent. Personally, I think it doesn't belong in the top bar.
  • And in fact, using a top bar for jumps within the same page feels a bit non-standard - can you think of a popular site that behaves this way? With how minimal our sections are at the moment, jump links displayed so prominently feels a bit overkill, especially as we already have a Download button at the top of the page as well.

@Toby-Rea
Copy link
Author

Toby-Rea commented Feb 2, 2025

Yeah I do agree with these points, I think pulling them out into a table of contents would resolve that, we can then reserve the top bar for external links and even feature a prominent download link there too. Regarding the background, I've applied styles specific to OS preferences with the dark: prefix. Once I've finished the light and dark colour palettes this won't be a problem.

@ittipatken
Copy link

  • can you think of a popular site that behaves this way?

https://www.microsoft.com/en-us/microsoft-365 does use top bar for navigating in the same page but I agree that most websites do not. Usually the top bar only contains external links to other pages.
image

  • With how minimal our sections are at the moment, jump links displayed so prominently feels a bit overkill, especially as we already have a Download button at the top of the page as well.

However, the Anki site has the true download links in the same page. I feel like adding a download button on the top bar just for jumping to the section is justifiable, additionally David suggests to show download button at all times.

@dae
Copy link
Member

dae commented Feb 3, 2025

Thank you for that Microsoft example, it's good to know - but agreed it seems atypical. I agree that we want Download displayed prominently. I don't have strong preferences whether that's the existing download button at the top, or it moved to the top bar.

@Toby-Rea
Copy link
Author

Toby-Rea commented Feb 5, 2025

I've made the adjustments we've discussed here, further feedback would be welcome. I ended up reneging on splitting the work for the table of contents, I'd rather not draw it out longer than required.

@Toby-Rea
Copy link
Author

Toby-Rea commented Feb 6, 2025

Most Recent:

image

image

image

@dae
Copy link
Member

dae commented Feb 16, 2025

Sorry for the delay, I have a backlog of GitHub notifications and forgot to check back here.

I think the top bar looks great.

The 'on this page' I'm a bit less sold on - when I first saw it mostly off the screen, I assumed it was a bug/unintended layout, and it was only when I hovered the cursor over it that I realised that it was intentional. I find myself wondering whether some people will just assume the former, and never realise they can hover over it.

When I realised you could hover over it, I found it does look slick. But the page is not particularly long - is this really adding enough value to justify the potential confusion? I'm open to a good argument for why it should stay.

@Toby-Rea
Copy link
Author

Sorry for the delay, I have a backlog of GitHub notifications and forgot to check back here.

That's okay, there's no rush with this project.

The 'on this page' I'm a bit less sold on - when I first saw it mostly off the screen, I assumed it was a bug/unintended layout, and it was only when I hovered the cursor over it that I realised that it was intentional. I find myself wondering whether some people will just assume the former, and never realise they can hover over it.

When I realised you could hover over it, I found it does look slick. But the page is not particularly long - is this really adding enough value to justify the potential confusion? I'm open to a good argument for why it should stay.

I actually started second-guessing this change pretty soon after I made it as well. I agree now that the page isn't really long enough to justify it's presence. Since we have that download button in the top navigation that's always there, I think we can safely remove it.

@dae dae merged commit 58e4536 into ankitects:main Feb 18, 2025
1 check passed
@Toby-Rea Toby-Rea deleted the navigation-redesign branch February 18, 2025 09:38
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.

3 participants