Skip to content

add support for navigation entries that contain a fragment identifier #183

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

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

mojavelinux
Copy link
Contributor

@RichardSmedley
Copy link
Contributor

Thanks @mojavelinux
Much appreciated!

@osfameron is out-of-office today, so I'll leave the merge for him tomorrow - but LGTM 👍

Copy link
Contributor

@osfameron osfameron left a comment

Choose a reason for hiding this comment

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

Thanks Dan! This does work with one minor niggle.

e.g.

  1. OK: Choose SDK 4.1, click on "Project Docs -> Release Notes"
    The nav menu syncs to the fragment. Great!

  2. STILL OK: from here, now choose SDK 4.3
    It looks like choosing a version from the dropdown loses the fragment, which in this case happens to be correct, and we arrive at the fragmentless URL, which matches nav, and syncs.

  3. NOT OK: Now choose SDK 4.1 again.
    This goes to the URL without the fragment.
    As the one in the Nav has a fragment, they now don't match, and the nav menu does NOT sync.

So, case 1 fixes the problem we reported, thanks!

Case 3 isn't great though. It'll be easy to follow the workflow of "Arrive on Current version, choose Release Notes, now switch to older version".

Do you need to support cases where the Nav menu links to the same URL multiple times at different fragments?
(If not, the simple fix of matching the URL only could fix this.
Potentially if you need to support the case, but we don't, we could add that simplified handling locally?)

Anyway, this is a great improvement, I'll merge, and invoke @RichardSmedley to advise on how big a problem the remaining issue is for him.

@osfameron osfameron merged commit 25c4ff3 into couchbase:master Jun 14, 2024
@mojavelinux
Copy link
Contributor Author

The navigation system makes an assumption that switching versions should not preserve the fragment. However, that could be added. It's not possible to drop the fragment, then restore it when switching back to a version since once it's gone, it's gone. It either has to be preserved or not preserved. It sounds like preserving would work best for you. I can come up with a PR to show how that is done.

@mojavelinux
Copy link
Contributor Author

Preserving the hash (aka URL fragment) on version change is a universal change. See #184

@mojavelinux mojavelinux deleted the nav-with-fragment branch July 3, 2024 19:27
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