Skip to content

Conversation

@nnelgxorz
Copy link
Contributor

@nnelgxorz nnelgxorz commented Jul 13, 2022

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

Exposes a tagName attribute on the <Host/> tag and a host:tagName attribute on Qwik Components.

This PR also adds a better type that is used for the tagName attribute, host:tagName and the tagName render option which gives users autocomplete for all html tags, while retaining the ability to use custom tag names as well.

ie, typing an m gives you autocomplete for main, map, mark, marquee, menu, meta, and meter. But misko, manu, and my-component are also valid!

Use cases and why

When building a design system, it is often desirable to reuse a component but render into a variety of tags. This is not currently possible because the only way to set a host element tag name is as an argument to component$ and not from within component$

Imagine a Header component that would be used like so: <Header as='h3' text='Hello!'/>

Within the component definition we can check the as attribute for different styles and set the tagName on the host element:

const Heading = component$((props: {as: 'h1' | 'h2' | 'h3'}) => {
    const styles = {
      h1: 'font-size: 3rem', 
      h2: 'font-size: 2.5rem', 
      h3: 'font-size: 2rem',
    };
    return <Host tagName={props.as} style={styles[props.as]}><Slot/></Host>
  });

and then use it like this:

<Heading as="h1"/>
<Heading as="h2"/>
<Heading as="h3"/>

Or a component that just renders as a 'pill'. Maybe we want a div, but maybe we want an li, dd, span, or any other valid HTML tag. With an as attribute we can have that flexibility. <Pill as='span'/> 🚀

const Pill = component$((props: { as: keyof HTMLElementTagNameMap }) => {
  return <Host tagName={props.as}><Slot/></Host>
})

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@manucorporat
Copy link
Contributor

Love this idea! but just not sure this might be a bit problematic, as "as" might be too generic.

How about host:as?

@nnelgxorz
Copy link
Contributor Author

Totally agree @manucorporat. There might also be confusion between as and the tagName component option.

Maybe leave as as a user defined property, that way it can be restricted to certain tags: h1, h2, h3, etc. and you could do host:tagName={props.as} and <Host tagName={props.as}>{...}</Host>

What's your feeling?

@nnelgxorz
Copy link
Contributor Author

const Heading = component$((props: {as: 'h1' | 'h2' | 'h3'}) => {
    const styles = {
      h1: 'font-size: 3rem', 
      h2: 'font-size: 2.5rem', 
      h3: 'font-size: 2rem',
    };
    return <Host tagName={props.as} style={styles[props.as]}><Slot/></Host>
  });

@nnelgxorz nnelgxorz changed the title Add 'as' attribute Expose tagName on <Host/> and host:tagName on Components Jul 13, 2022
@nnelgxorz nnelgxorz marked this pull request as ready for review July 13, 2022 22:48
@nnelgxorz
Copy link
Contributor Author

nnelgxorz commented Jul 13, 2022

I noticed that creating a component that visually appears as a button but can be either a link or a button depending on if an href is present:

const Button = component$((props: {href?: string}) => {
  const tagName: keyof HTMLElementTagNameMap = props.href ? 'a' : 'button';
  return <Host tagName={tagName} href={props.href} ><Slot/></Host>
})

Won't typecheck because <Host> only accepts base HTML attributes. This does typecheck:

const Button = component$((props: {href?: string}) => {
  const tagName: keyof HTMLElementTagNameMap = props.href ? 'a' : 'button';
  return <Host tagName={tagName} {...props} ><Slot/></Host>
})

I think the first example is a valid use case, but I'd be interested to hear thoughts before I look at making changes. 👍

@manucorporat
Copy link
Contributor

No! i was looking into this feature the other day since the Builder SDK for Qwik will need something very similar, RenderBlocks acting as any html tag, reviewing PR!

@manucorporat
Copy link
Contributor

manucorporat commented Jul 14, 2022

I noticed that creating a component that visually appears as a button but can be either a link or a button depending on if an href is present:

const Button = component$((props: {href?: string}) => {
  const tagName: keyof HTMLElementTagNameMap = props.href ? 'a' : 'button';
  return <Host tagName={tagName} href={props.href} ><Slot/></Host>
})

Won't typecheck because <Host> only accepts base HTML attributes. This does typecheck:

const Button = component$((props: {href?: string}) => {
  const tagName: keyof HTMLElementTagNameMap = props.href ? 'a' : 'button';
  return <Host tagName={tagName} {...props} ><Slot/></Host>
})

I think the first example is a valid use case, but I'd be interested to hear thoughts before I look at making changes. 👍

I am not sure how this is going t work... since currently the tagName needs to be known before the component logic even runs... this is due to the fact that we need a HostElement before the component renders...

I can see we could support:

<MyCmp host:tagName="thing">

but not sure how we can support <Host tagName="thing"> and dynamic tag name... without a massive refactor

@nnelgxorz
Copy link
Contributor Author

I am not sure how this is going t work... since currently the tagName needs to be known before the component logic even runs... this is due to the fact that we need a HostElement before the component renders...

I can see we could support:

<MyCmp host:tagName="thing">

but not sure how we can support <Host tagName="thing"> and dynamic tag name... without a massive refactor

Fair point. Maybe something to revisit later down the road. If you're happy I'll fix the build error so you can merge! 🚀

@manucorporat
Copy link
Contributor

Sounds good! but the current PR is not working right? i dont see the part of the logic that makes it work, I am confused :)

@manucorporat
Copy link
Contributor

ohh! it's exploiting the _tagname property form qwik-dom? problem is that this will not work when the component is mounted in the client :(

@manucorporat
Copy link
Contributor

In the dom, it's not possible to change the tag name of a DOM node

@nnelgxorz
Copy link
Contributor Author

I wasn't sure if that was the best way to do it, but it got my tests passing. Is there are better place to catch that?

mhevery
mhevery previously approved these changes Jul 14, 2022
@mhevery mhevery self-requested a review July 14, 2022 20:11
@mhevery
Copy link
Contributor

mhevery commented Jul 14, 2022

Sorry, accidentally hit Approve, I am leaving this to @manucorporat

@mhevery mhevery dismissed their stale review July 14, 2022 20:11

Accidently approved

@manucorporat
Copy link
Contributor

So, i like the idea of this PR, and we will need this feature, however, the current way this implemented... is very implementation dependant in (qwik-dom) Domino, which might be replaced at some point. Having a feature that we cant port.

the only way I can see this being implemented in by allowing:

<MyCmp host:as="tagName"> 

or

<MyCmp host:tagName="tagName"> 

then in the componentQrl function do this:

export const componentQrl = <PROPS extends {}>(
  onRenderQrl: QRL<OnRenderFn<PROPS>>,
  options: ComponentOptions = {}
): Component<PROPS> => {
  const tagName = options.tagName ?? 'div';
  const skipKey = ELEMENTS_SKIP_KEY.includes(tagName);

  // Return a QComponent Factory function.
  return function QSimpleComponent(props, key): JSXNode<PROPS> {
    const finalTag = props['host:tagName'] ?? tagName
    const finalKey = skipKey ? undefined : onRenderQrl.getHash() + ':' + (key ? key : '');
    return jsx(finalTag, { [OnRenderProp]: onRenderQrl, ...props }, finalKey) as any;
  };
};

@nnelgxorz
Copy link
Contributor Author

Sounds good @manucorporat. I'll try to make the changes this afternoon/evening. I still think the design system use case is important, but if it's out of reach for now I'm happy to leave it as an enhancement somewhere in the future. 👍

@manucorporat
Copy link
Contributor

I think you can still implement your idea for the design system, but you will have to create your own component$() that redirects the as prop

@nnelgxorz
Copy link
Contributor Author

Got it scaled back to just host:tagName and moved the implementation into componentQrl.

Let me know if I need to make any other changes. 👍

@nnelgxorz nnelgxorz changed the title Expose tagName on <Host/> and host:tagName on Components Expose host:tagName on Components Jul 22, 2022
@mhevery mhevery merged commit bf5cd4f into QwikDev:main Jul 22, 2022
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.

3 participants