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

feat: functional router-link, fixed #2021 #2029

Closed
wants to merge 7 commits into from
Closed

feat: functional router-link, fixed #2021 #2029

wants to merge 7 commits into from

Conversation

QingWei-Li
Copy link

@miljan-aleksic
Copy link
Contributor

Awesome @QingWei-Li, just seems the TS test didn't pass. Can you check that?

@posva
Copy link
Member

posva commented Jan 29, 2018

It looks like it's missing a type annotation for the destructured param. I'll give it a look but feel free to push any extra commits if you find the solution before I can give it a look 🙂

edit: you can also set the type as any for the moment

@QingWei-Li QingWei-Li changed the title feat: funtional router-link, fixed #2021 feat: functional router-link, fixed #2021 Jan 29, 2018
@miljan-aleksic
Copy link
Contributor

The functional conversion seems fine and perhaps we could, after the proper tests, merge that as step 1.

As step 2 we should see how to merge the props with the data for the final component.

@posva
Copy link
Member

posva commented Jan 29, 2018

About the merging: it should be ok since the router-link component is already merging the attributes it modifies (classes and attr mostly)

@miljan-aleksic
Copy link
Contributor

Yes but that doesn't cover custom props. Ideally when setting a custom tag, router-link should behave as the component itself and accept any custom prop. I am not seeing anything that covers that, am I missing it?

@QingWei-Li
Copy link
Author

QingWei-Li commented Jan 30, 2018

@miljan-aleksic All props are in the data, I just extended the data.class, other props will be inherited by the custom component.

@posva
Copy link
Member

posva commented Feb 27, 2018

Sorry for the delay. Before merging this I was wondering if this could have a negative impact on performance if there're too many routes. Maybe we could cache the resolve in the future but I haven't even checked if it's worth doing

@miljan-aleksic
Copy link
Contributor

It may have small performance differences but wouldn't be those routes only rerendered if the parent does? So for example, if you wrap a long list of routes with a standard component those would get the cache benefits from it, right?

@posva
Copy link
Member

posva commented Feb 28, 2018 via email

@miljan-aleksic
Copy link
Contributor

Maybe those edge situations are the ones that should be solved by wrapping router-link in a stateful component :)

@miljan-aleksic
Copy link
Contributor

@posva, I have run into this issue again, is not so isolated as it seems. What can we do to push this forward?

@posva
Copy link
Member

posva commented Apr 10, 2018

Looking again into this, I added a test to check class and style merging happens correctly as well as other tests for #2121
I will need to change that test into a unit one in the future though

@posva
Copy link
Member

posva commented Apr 10, 2018

This will also close #2021 and #1572

@miljan-aleksic
Copy link
Contributor

Any hopes about this getting merged? :)

@yyx990803
Copy link
Member

Making <router-link> functional is going to be a breaking change, so this is not going to happen in router 3.x.

To more completely solve this use case here, this is what we are thinking for router v4:

  • <router-link> no longer renders an element of itself, instead, the user must provide the element to be rendered via normal slots or scoped slots.
<!-- simple usage: normal slot -->
<!-- implicitly add @click and :href to <a> -->
<router-link to="...">
  <a>Hello</a>
</router-link>

<!-- advanced usage: scoped slot -->
<!-- explicitly get accress to resolved href and click handler and bind yourself -->
<router-link to="..." self-slot-scope="{ href, navigate }">
  <my-button @click="navigate">
    <i slot="icon">x</i>
  </my-button>
</router-link>

@miljan-aleksic
Copy link
Contributor

@yyx990803, what is a self-slot-scope? Some new concept?

Perhaps an intermeddiate router-link-scope component could fill the gap for the v3. No braking changes while providing some useful feedback for v4.

@posva
Copy link
Member

posva commented Jul 2, 2018

Think about self-slot-scope as a shorter version from:

<router-link to="...">
  <template slot-scope="{ href, navigate }">
    <my-button @click="navigate">
      <i slot="icon">x</i>
    </my-button>
  </template>
</router-link>

but in this scenario, a slot-scope on my-button should also work

@miljan-aleksic
Copy link
Contributor

I never saw it, it is a new feature for next vue?

@posva
Copy link
Member

posva commented Jul 2, 2018

It's not something that exists yet, it's not the final api either, so don't worry much about that 🙂

@posva
Copy link
Member

posva commented Apr 20, 2020

@QingWei-Li Thanks a lot for the PR. Given that this change would be a breaking change and that the v-slot api cover the use cases that were initially described at #2021, I recommend to use that when dealing with custom components

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants