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

fix: a more robust aria-current="page" algorithm for A #2770

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

metatoaster
Copy link
Contributor

@metatoaster metatoaster commented Aug 5, 2024

This started with the fix to setting of aria-current="page" for href that ends with a single trailing slash, such as <A href="/item/">, during navigation to it, and further testing found additional problems with the initial algorithm which was introduced in #1656 (which was a much appreciated starting point). Work was done to refactor the std::iter::zip call into its own function and extended it to support the initial desired use-case, along with multi-level matching, and includes a decent number of test cases with a fairly exhaustive set of href/location combination (which lead to the discovery of the issue with multi-level matching). There are some edge-case tests also, though given the nature of that less common interpretation I have not include the ability to allow the configuration of that for the helper function. I imagine if this is needed a future change can just be a boolean flag to determine the match condition when the final href_f fragment is Some("") - further details on how this all works are left as comments in the relevant sections of this pull request.

This pull request was re-submitted as per discussion at #2744.

Edit: I might actually go ahead and implement this as a flag with this pull request if needed, since shortly after I wrote that I saw the path on how to actually do this (and came up with the name to the flag). In any case this additional change will be a separate commit so if it is required to be part of a separate pull request we can have this done easily.

metatoaster added a commit to metatoaster/leptos that referenced this pull request Aug 5, 2024
- Anchors with href ending with a single trailing slash (e.g. `<A
  href="/item/">`) will now get the `aria-current="page"` marker set
  when navigating under its subpaths such as `/item/1`.
- Also fixes multi-level matching.
- Refactored the check to its own function.
- Include test cases for the algorithm.
- Fixes leptos-rs#2744 and leptos-rs#2770
@metatoaster metatoaster force-pushed the anchor_current_trailing_slash branch from 8c887b6 to bfe4ac4 Compare August 5, 2024 00:05
@metatoaster
Copy link
Contributor Author

metatoaster commented Aug 5, 2024

Went ahead and did it, but turns out there can be another scenario where only the immediate parent should be highlighted, and now there's way more possible combination which will complicate things if pursued further, so I am taking a break from this problem with these two changesets. Please let me know if I should split the second change into a separate pull request to be considered in another time, otherwise I am fine with them being merged as is, though the naming of the new argument may be lacking as naming things good is hard.

metatoaster added a commit to metatoaster/leptos that referenced this pull request Aug 5, 2024
- Anchors with href ending with a single trailing slash (e.g. `<A
  href="/item/">`) will now get the `aria-current="page"` marker set
  when navigating under its subpaths such as `/item/1`.
- Also fixes multi-level matching.
- Refactored the check to its own function.
- Include test cases for the algorithm.
- Fixes leptos-rs#2744 and leptos-rs#2770
@metatoaster metatoaster force-pushed the anchor_current_trailing_slash branch from bac890d to 4c5221e Compare August 5, 2024 11:49
metatoaster added a commit to metatoaster/leptos that referenced this pull request Aug 7, 2024
- Anchors with href ending with a single trailing slash (e.g. `<A
  href="/item/">`) will now get the `aria-current="page"` marker set
  when navigating under its subpaths such as `/item/1`.
- Also fixes multi-level matching.
- Refactored the check to its own function.
- Include test cases for the algorithm.
- Fixes leptos-rs#2744 and leptos-rs#2770
@metatoaster metatoaster force-pushed the anchor_current_trailing_slash branch from 4c5221e to cf0b889 Compare August 7, 2024 15:42
@metatoaster
Copy link
Contributor Author

Both commits should pass the cargo fmt step now.

- Anchors with href ending with a single trailing slash (e.g. `<A
  href="/item/">`) will now get the `aria-current="page"` marker set
  when navigating under its subpaths such as `/item/1`.
- Also fixes multi-level matching.
- Refactored the check to its own function.
- Include test cases for the algorithm.
- Fixes leptos-rs#2744 and leptos-rs#2770
- Implement handling of situation where a non-root href with a trailing
  slash only matching exact trailing slash and not the other version,
  for example, an `href="/item/"':

  - location="/item" will not match;
  - location="/item/" will match, and thus aria-current="page" set.

- Relevant comment in test has been amended to suggest that there may be
  another solution where only the immediate parent location should have
  the flag set.
@metatoaster metatoaster force-pushed the anchor_current_trailing_slash branch from cf0b889 to 014034a Compare August 8, 2024 03:54
@gbj
Copy link
Collaborator

gbj commented Aug 10, 2024

Thanks! Failing clippy (see its suggestion, and I'd always suggest using clippy in your editor with rust-analyzer) but the test cases look good to me so presumably if it's passing these I will merge.

@gbj gbj merged commit 9f99571 into leptos-rs:main Aug 11, 2024
57 of 67 checks passed
chrisp60 pushed a commit to chrisp60/leptos that referenced this pull request Aug 12, 2024
chrisp60 pushed a commit to chrisp60/leptos that referenced this pull request Aug 12, 2024
chrisp60 pushed a commit to chrisp60/leptos that referenced this pull request Aug 12, 2024
chrisp60 pushed a commit to chrisp60/leptos that referenced this pull request Aug 12, 2024
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.

2 participants