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: HomeHero NavLink reactivity lost (#195) #212

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

ShenQingchuan
Copy link
Contributor

Please see #195 for more details... (May Chinese required)

Copy link
Member

@kiaking kiaking left a comment

Choose a reason for hiding this comment

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

Thank you so much for the PR! I've left a comment so please have a look. I think we only need to change NavLink.vue with this change.

@kiaking kiaking added the bug Something isn't working label Feb 10, 2021
@kiaking kiaking changed the title fix: HomeHero NavLink reactivity lost. fix: HomeHero NavLink reactivity lost (#195) Feb 10, 2021
@ShenQingchuan
Copy link
Contributor Author

Yes. Without toRefs it still works, that's my mistake...

Copy link
Member

@kiaking kiaking left a comment

Choose a reason for hiding this comment

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

I don't think we need NavDropdownLinkItem.vue and navLink.ts changes anymore. Can we delete them? 👀

@ShenQingchuan
Copy link
Contributor Author

I don't think we need NavDropdownLinkItem.vue and navLink.ts changes anymore. Can we delete them? 👀

Sure, I'll refactor my commits and repush later...

@ShenQingchuan
Copy link
Contributor Author

OOPS !! @_@ @kiaking

navLinks.ts need to be changed...

Why I need the argument item of useNavLink(item) to be a Ref<DefaultTheme.NavItemWithLink>:

Because the value you passed into in HomeHero.vue as item v-bind:

<NavLink
  v-if="hasAction"
  :item="{ link: data.actionLink, text: data.actionText }"
  class="action"
/>

is Not a ref at all, when the locales changed, the data(from frontMatter info) did change, but the object itself didn't change at all, it performs as a constant object.

cut a long story short, it will show a bug when you follow these steps:

  1. open a English version doc first
  2. click another language locale link
  3. and you can see the homehero button's text has changed, that's fine and what we expected.
  4. but if you click the button, then we will still go to the English section, not the language we wanted.

@ShenQingchuan
Copy link
Contributor Author

the item passed it into useNavLink need to be a ref, or it will never changed...

you can set a watch and write a function to console.log(item.link) for debuging,

and you'll see the item.link string value has never changed to anthoer language (such as 'zh') /zh/xxx but still /xxx

@ShenQingchuan
Copy link
Contributor Author

ShenQingchuan commented Feb 10, 2021

I did this PR a long time ago, I just didn't remember some details at conversations below ,,, feel really embarrassed now to bring you more chaos ... Sorry 🙏🏻

For now I think we should keep the original version of my PR... 😅😅

@kiaking
Copy link
Member

kiaking commented Feb 11, 2021

Ah good point! Yap. To note, it's not because of how we pass props, props are reactive because item.text is working just fine. It's about how we're passing props to the composable function. Maybe we should have made it call it from the parent component. I might refactor this later.

But for now, thank you so much for the fix!

@kiaking kiaking merged commit 5678dc3 into vuejs:master Feb 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants