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

[2.x] Allow custom component on Link "as" prop in React, Vue2 and Vue3 #1940

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

guizoxxv
Copy link

@guizoxxv guizoxxv commented Aug 11, 2024

This PR adds the ability to use a custom component in the as prop of the Link component. Currently it only accepts a string value. The proposed changes would allow to use the link as:

  • Vue 3
<script setup>
import { Link } from '@inertiajs/vue3';
import CustomComponent from '../Components/CustomComponent.vue';
</script>

<template>
  <div>
    <Link href="/article" :as="CustomComponent">
      Custom Component Link
    </Link>
    <Link href="/article" as="button">
      HTML Tag Link (button)
    </Link>
  </div>
</template>
  • Vue 2
<script setup>
import { Link } from '@inertiajs/vue2';
import CustomComponent from '../Components/CustomComponent.vue';
</script>

<template>
  <div>
    <Link href="/article" :as="CustomComponent">
      Custom Component Link
    </Link>
    <Link href="/article" as="button">
      HTML Tag Link (button)
    </Link>
  </div>
</template>

Because Link is a Functional Component it is necessary to manually pass attributes and listeners in the CustomComponent - https://v2.vuejs.org/v2/guide/render-function#Passing-Attributes-and-Events-to-Child-Elements-Components.

  • React
import { Link } from '@inertiajs/react';
import CustomComponent from '../Components/CustomComponent';

const Home = () => {
  return (
    <>
        <Link href="/article">
          HTML Tag Link (button)
        </Link>
        <Link href="/article" as={CustomComponent}>
          Custom Component Link
        </Link>
    </>
  );
};

export default Home;

This feature was requested in #1550, #1654, #1668 and #1746.

@guizoxxv guizoxxv changed the title Allow custom component on Link "as" prop in React and Vue3 Allow custom component on Link "as" prop in React and Vue2 and Vue3 Aug 11, 2024
@guizoxxv guizoxxv changed the title Allow custom component on Link "as" prop in React and Vue2 and Vue3 Allow custom component on Link "as" prop in React, Vue2 and Vue3 Aug 11, 2024
@guizoxxv guizoxxv marked this pull request as ready for review August 11, 2024 07:06
@pedroborges pedroborges self-assigned this Sep 4, 2024
@pedroborges pedroborges added the enhancement New feature or request label Sep 4, 2024
@pedroborges
Copy link
Collaborator

Thanks @guizoxxv, I'll test your implementation and get back to you soon.

Copy link
Collaborator

@pedroborges pedroborges left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I tested it today and everything works smoothly except for one small accessibility issue. When a custom component passed to the as prop renders an anchor tag, it doesn't receive the href attribute, which breaks accessibility:

<Link :as="CustomAnchor" href="/" class="hover:underline">Home</Link>

<a class="hover:underline">Home</a>

The link still functions due to the onclick handler, but without the href, it’s not fully accessible. Could you ensure that the href attribute is passed when the custom component renders an anchor element?

@pedroborges pedroborges removed their assignment Sep 12, 2024
@pedroborges pedroborges added react Related to the react adapter vue 3 Related to the vue 3 adapter labels Sep 13, 2024
@pedroborges pedroborges changed the title Allow custom component on Link "as" prop in React, Vue2 and Vue3 [2.x] Allow custom component on Link "as" prop in React, Vue2 and Vue3 Sep 13, 2024
@guizoxxv
Copy link
Author

guizoxxv commented Sep 14, 2024

@pedroborges Thanks for the review.

I didn't find an effective way to discover the HTML element of the custom component main node without mounting it. Something like this:

const getCustomComponentTagName = (customComponent: Component): string => {
  const app = createApp(customComponent);
  const instance = app.mount(document.createElement('div'));
  const tagName = instance.$el.tagName; // single root component example
  app.unmount();

  return tagName;
}

I'm not sure if this temporary rendering would be viable considering performance/resources. In that case, do you know an altertive way?

@pedroborges
Copy link
Collaborator

@guizoxxv Yeah, this solution is not good for performance. Here’s an alternative approach that should be more performant:

  • Always pass href to the custom components.
  • Store the rendered component in a ref to access the DOM element once it's rendered.
  • Use nextTick to wait until the component is fully rendered, then check its tagName.
  • If the tagName is not A, simply remove the href attribute.

This way, you can avoid unnecessary temporary rendering and still maintain accessibility when an anchor element is used. What do you think of this approach? Let me know if you need help implementing it 😉

@guizoxxv
Copy link
Author

You mean something like this (Vue3 example)?

setup(props, { slots, attrs }) {
  const elementRef = ref(null)

  nextTick(() => {
    if (elementRef.value) {
      const element = elementRef.value.$el

      if (element.tagName !== 'A') {
        element.removeAttribute('href')
      }
    }
  })

  return () => {
    const isAnchor = props.as === 'a' || props.as === 'A'
    const isCustomComponent = typeof props.as !== 'string'
    const method = props.method.toLowerCase() as Method
    const [href, data] = mergeDataIntoQueryString(method, props.href || '', props.data, props.queryStringArrayFormat)

    if (isAnchor && method !== 'get') {
      console.warn(
        `Creating POST/PUT/PATCH/DELETE <a> links is discouraged as it causes "Open Link in New Tab/Window" accessibility issues.\n\nPlease specify a more appropriate element using the "as" attribute. For example:\n\n<Link href="${href}" method="${method}" as="button">...</Link>`,
      )
    }

    return h(
      props.as,
      {
        ...attrs,
        ...(isAnchor || isCustomComponent ? { href } : {}),
        ...(isCustomComponent ? { ref: elementRef } : {}),
        onClick: (event) => {
          if (shouldIntercept(event)) {
            event.preventDefault()

            router.visit(href, {
              data: data,
              method: method,
              replace: props.replace,
              preserveScroll: props.preserveScroll,
              preserveState: props.preserveState ?? method !== 'get',
              only: props.only,
              except: props.except,
              headers: props.headers,
              // @ts-expect-error
              onCancelToken: attrs.onCancelToken || (() => ({})),
              // @ts-expect-error
              onBefore: attrs.onBefore || (() => ({})),
              // @ts-expect-error
              onStart: attrs.onStart || (() => ({})),
              // @ts-expect-error
              onProgress: attrs.onProgress || (() => ({})),
              // @ts-expect-error
              onFinish: attrs.onFinish || (() => ({})),
              // @ts-expect-error
              onCancel: attrs.onCancel || (() => ({})),
              // @ts-expect-error
              onSuccess: attrs.onSuccess || (() => ({})),
              // @ts-expect-error
              onError: attrs.onError || (() => ({})),
            })
          }
        }, 
      },
      slots,
    )
  }
},

I like it. Another idea I had is adding a new prop for this scenario, for example keep-href:

<Link :as="CustomAnchor" href="/" keep-href>Home</Link>

However, this add more burden to the user, while yours don't. Also, it feels counter intuitive that without it, the href would be removed. I think yours is best, let me know if the implementation in Vue3 is correct and I'll try to replicate in the others.

@pedroborges
Copy link
Collaborator

This sounds like a good plan! I’m with you on not adding extra burden on the user. Also, I think we might not even need nextTick here. Since we're dealing with DOM elements, onMounted might already do the job.

You could try moving the nextTick logic into onMounted and see if it works just as well. It should simplify things further.

@guizoxxv
Copy link
Author

guizoxxv commented Sep 17, 2024

Yeah, I just bumped into another problem. In Vue 2, because the Link is a functional component, which is stateless, I cannot use lifecycle hooks.

onMounted hook worked fine in replacement for nextTick in Vue 3.

@pedroborges
Copy link
Collaborator

pedroborges commented Sep 17, 2024

@guizoxxv Vue 2 supports lifecycle hooks in a different syntax, see the mounted method.

@guizoxxv
Copy link
Author

guizoxxv commented Sep 17, 2024

@pedroborges
My understanding is that functional components are stateless, they do not work with reactive data and don't have access to lifecycle hooks - https://v2.vuejs.org/v2/guide/render-function#Functional-Components.

Typescript interface FunctionalComponentOptions (used in Vue 2 Link component) does not contain lifecycle hooks. I removed this type just for testing, added the mounted hook and it didn't do anything. After changing functional to false it worked.

@pedroborges
Copy link
Collaborator

Thanks for the clarification, @guizoxxv, and my apologies for the confusion! Functional components wasn't a thing when I last worked with Vue 2, so I wasn’t fully aware of their limitations.

That being said, I also forgot to mention that we’re dropping support for Vue 2 in Inertia V2. Since your PR introduces new behavior, we'll be merging it into V2. For now, you can go ahead and make these changes just for the React and Vue 3 adapters. Once the V2 branch is live, we’ll rebase and merge your work.

Thanks again for your contribution, and I appreciate your patience!

@guizoxxv
Copy link
Author

No problem. I had more experience with Vue 2 - only started with Vue 3 last year =). It makes sense to drop support for Vue 2 since it reached end of life. I reverted my code to keep the Vue 2 file as it was.

I had a bit of a strugle in React due to the ref type, which can be a function. I ended up using this function combinedRef, but I'm not sure if is the best approach. Also, it requires the user to foward the ref in the custom component. Example:

import { forwardRef, Ref } from "react"

const CustomComponent = forwardRef(({ children, ...props }, ref: Ref<HTMLButtonElement>) => {
  return (
    <button ref={ref} {...props}>
      {children}
    </button>
  )
})

export default CustomComponent

@pedroborges
Copy link
Collaborator

Thanks, @guizoxxv! We’ve been focusing on merging bug fixes for 1.3 this week, but I’ll make sure to review your changes next week. Appreciate your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request react Related to the react adapter vue 3 Related to the vue 3 adapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants