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

Transform item's path to lowercase in navigator #836

Merged
merged 4 commits into from
May 29, 2024

Conversation

marinaaisa
Copy link
Member

@marinaaisa marinaaisa commented May 24, 2024

Bug/issue #122911237, if applicable:

Summary

Transform item's path to lowercase in navigator, so if symbols have a path with capital letters, the highlighting of the active element in the navigator continues working

Dependencies

NA

Testing

Steps:

  1. Run a doccarchive with a navigator
  2. Make sure that everything works as expected

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

@marinaaisa marinaaisa requested review from mportiz08 and hqhhuang May 24, 2024 18:25
@@ -89,7 +89,7 @@
:is="refComponent"
:id="item.uid"
:class="{ bolded: isBold }"
:url="isGroupMarker ? null : (item.path || '')"
:url="isGroupMarker ? null : (item.path.toLowerCase() || '')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of changing the URL itself, maybe we can just add a new CSS class when item.path.toLowerCase() === this.$route.path.toLowerCase() that we apply the active/selected background color to?

That way we get the correct highlight/selection background color for the active item, regardless of whether the case matches between the two items, but we don't actually change the URL itself?

I think we investigated a similar problem with the tutorial dropdown in the past: #11

Copy link
Member Author

@marinaaisa marinaaisa May 27, 2024

Choose a reason for hiding this comment

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

Yes, it sounds like a better approach. I changed it.

It's a pity that vuejs/vue-router#3657 didn't get merged, it made sense to me.

@marinaaisa marinaaisa force-pushed the r122911237/item-path-lowercase branch from 82c3a22 to fd27de3 Compare May 27, 2024 14:12
@marinaaisa
Copy link
Member Author

@swift-ci test

@marinaaisa marinaaisa requested a review from mportiz08 May 27, 2024 14:13
@marinaaisa
Copy link
Member Author

@swift-ci test

@mportiz08 mportiz08 merged commit bd891e5 into swiftlang:main May 29, 2024
1 check passed
@mportiz08 mportiz08 deleted the r122911237/item-path-lowercase branch May 29, 2024 19:48
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