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

[DO NOT MERGE] [Bug Fix] navigation dropdown bold issue(SR-15313) #11

Closed
wants to merge 2 commits into from

Conversation

Kyle-Ye
Copy link
Collaborator

@Kyle-Ye Kyle-Ye commented Oct 27, 2021

Issue SR-15313

Summary

  • Primary tutorial navigation dropdown fails to bold the current tutorial when browser URL is not lowercased (SR-15313)
    • The "isActive" from router-link will be false since it tricks "/about" and "/abouT" are not active. And it seems hard for us to change the override the compare logic behind router-link to a case-insensitive one.
    • So I use the same method in MobileDropdown.vue to check "currentOption === ⌥.title" as isActive condition
// vue-router version 3.5.3 /src/components/link.js line 79
classes[exactActiveClass] = isSameRoute(current, compareTarget, this.exactPath)
classes[activeClass] = this.exact || this.exactPath
  ? classes[exactActiveClass]
  : isIncludedRoute(current, compareTarget)
// vue-router version 3.5.3 /src/utils/route.js line 119
export function isIncludedRoute (current: Route, target: Route): boolean {
  return (
    current.path.replace(trailingSlashRE, '/').indexOf(
      target.path.replace(trailingSlashRE, '/')
    ) === 0 &&
    (!target.hash || current.hash === target.hash) &&
    queryIncludes(current.query, target.query)
  )
}
  • Mobile tutorial navigation dropdown fails to list its child when browser URL is not lowercased
    • Change the if check to tutorialUrl === $route.path.toLowerCase() to make sure the condition is true when navigating to an UPPER-CASE or Mix-case one.

Testing

Navigating to: http://localhost:8000/tutorials/slothcreator/CREATING-CUSTOM-SLOTHS in the attached documentation catalog.(See the SR-15313's attachment) in normal mode and mobile mode(width less than some value will enter this mode) respectively.

Checklist

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

  • Ran npm test, and it succeeded
    [Vue warn]: Error in render: "TypeError: Cannot read property 'toLowerCase' of undefined"

Primary tutorial navigation dropdown fails to bold the current tutorial when browser URL is not lowercased (SR-15313)

Mobile tutorial navigation dropdown fails to list its child when browser URL is not lowercased
@Kyle-Ye Kyle-Ye marked this pull request as ready for review October 27, 2021 16:09
Copy link
Contributor

@dobromir-hristov dobromir-hristov left a comment

Choose a reason for hiding this comment

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

Hello there. Thank you for your contribution.

The fix you are proposing should work, but I think it would be better if we can address this at the source, and just open a PR on VueRouter to fix their comparison - https://github.com/vuejs/vue-router/blob/084e7785f3eb9364d3a0c26ab68358f6cf142e21/src/util/route.js#L119?

It looks straight forward, but there might be things I am overlooking. Vue Router already supports case-insensitive matching for routes themselves, so it should be good to fix route matching as well.

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 28, 2021

Hello there. Thank you for your contribution.

The fix you are proposing should work, but I think it would be better if we can address this at the source, and just open a PR on VueRouter to fix their comparison - vuejs/vue-router@084e778/src/util/route.js#L119?

It looks straight forward, but there might be things I am overlooking. Vue Router already supports case-insensitive matching for routes themselves, so it should be good to fix route matching as well.

Open a issue on vuejs/vue-router#3656
Pull Request on vuejs/vue-router#3657

@Kyle-Ye Kyle-Ye changed the title [Bug Fix] navigation dropdown bold issue(SR-15313) [DO NOT MERGE] [Bug Fix] navigation dropdown bold issue(SR-15313) Nov 11, 2021
mportiz08 added a commit to mportiz08/swift-docc-render that referenced this pull request Mar 23, 2022
Remove 'No Overview' text from `Description` component

Fixes: rdar://89691926
@Kyle-Ye Kyle-Ye closed this Apr 16, 2023
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.

None yet

2 participants