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

Link modifier? #333

Open
gossi opened this issue May 6, 2020 · 6 comments
Open

Link modifier? #333

gossi opened this issue May 6, 2020 · 6 comments

Comments

@gossi
Copy link
Collaborator

gossi commented May 6, 2020

I was wondering if a modifier for situations like this would be a good idea:

<MyComponentWhereIDontKnowTheElementFor {{link ...}}/>

the {{link}} modifier would turn the element into an <a> tag and apply its values. Passing around the link helper as primitive would still work as the modifier would take the helper as valid input although it really looks stupid: {{link (link ...)}} 😂

The idea: With only the helper each component need to be made aware of a link passed in as helper and then handle the logic of applying it or pass it down. The modifier eliminates this component-specific-handling and applies it to where it is attached, some examples:

<Button>foo</Button> 
-> <button class="_button1234">foo</button>

<Button {{link ...}}>foo</Button> 
-> <a href="..." class="_button1234">foo</a>

<Card>bar</Card> 
-> <div class="_card1744">bar</div>

<Card {{link ...}}>bar</Card> 
-> <a href="..." class="_card1744">bar</a>

WDYT?

@buschtoens
Copy link
Owner

An element modifier cannot simply change the element tag name and I don't think that this is desirable, since it's quite magical and non-obvious / unexpected. In your template you'd write:

<div {{link ...}}>...</div>

But the result would be:

<a href="...">...</a>

There is ember-element-helper already, which was made exactly for this job.

In general, I recommend to treat ember-link as a first class primitive in your app architecture that is on-par with click actions. You could create a common base link button component, that accepts an optional @onClick and optional @link argument and changes its tag accordingly and wires up the arguments. We could think about adding something like this instead:

<OptionalButton @nonInteractiveTagName="div" />
➡️ <div></div>

<OptionalButton @nonInteractiveTagName="div" @onClick={{fn ...}} />
➡️ <button></button>	

<OptionalButton @nonInteractiveTagName="div" @link={{link ...}} />
➡️ <a href="..."></a>

<OptionalButton @nonInteractiveTagName="div" @link={{link ...}} @onClick={{fn ...}} />
➡️ <a href="..."></a>

@gossi
Copy link
Collaborator Author

gossi commented May 30, 2020

I made a PoC to see if changing the element would work. Well it does to what I expected it to do: https://ember-twiddle.com/b8b77f000ae29d52534b10936c4c4674?openFiles=templates.application%5C.hbs%2C&route=%2Fpage-1

See the difference between the links on the top and the bottom. The ones on the bottom ignore the listeneres attached with the {{on}} modifier. Dunno what's the order of execution of modifiers here? (perhaps by registration order in the modifier manager). There are solutions to that:

  • Make sure {{link}} is executed first - perhaps needs its own modifier manager
  • Go low-level as {{element}} does and do an AST transformation here, while {{link}} modifier is then only responsible for executing transitionTo()
  • Do some weird dom magic, that is very much likely to break or miss things.

That said, I haven't discarded the idea. Because of the nice symmetry of building blocks the modifier and helper would offer: One to pass it around, the other to put it onto the element (with its clear purpose of changing the element). That would eliminate the fact, that components need to be made aware of taking a @llink arg and can handle it or not.

@cah-brian-gantzler
Copy link

I would think the desired action would be achieved having the current component support an inline version that instead of yields does tag. with appropriate use of ...attributes it should accomplish the same thing.

I would vote of a default implementation when not yeilding.

@buschtoens
Copy link
Owner

buschtoens commented Sep 23, 2020

@gossi FWIW I added an attribute to the switched element you applied the modifier to to show that it breaks the internal Glimmer references and templates updates. Look at the tags in the DevTools and check the data-counter attribute that gets incremented when clicking the button. Twiddle

I really don't think that we should try to hack this deep into the Glimmer internals. If there's no public API way, we shouldn't do it.

@cah-briangantzler Hm, when I think this through it doesn't seem to make sense. I'd like to confirm, that I understood you right. You're proposing allowing a block-less invocation like this:

<Link
  @route="some.route"
  @models={{array 123}}
  @query={{hash foo="bar"}}
/>

And this should then yield some default HTML, which I guess should be an <a ...attributes> tag? Immediate questions that arise for me are:

  • How do I specify the innerHTML content of the <a> content here </a>?
  • How does this solve the <a> vs <button> (vs non-interactive) duality? <Link> currently does not accept an @onClick arg, nor does it allow to be invoked without the required param @route?

@buschtoens
Copy link
Owner

Coming back to the <OptionalButton> or maybe rather <LinkButton> (or whatever name 😅) from #333 (comment). The implementation would be straightforward and could even be done as a template-only component:

{{#if @link~}}
  <a
    href={{@link.url}}
    {{on "click" (if @replace @link.replaceWith @link.transitionTo)}}
    {{on "click" (optional @onClick)}}
    ...attributes
  >
    {{~yield~}}
  </a>
{{~else if @onClick~}}
  <button {{on "click" @onClick}} ...attributes>
    {{~yield~}}
  </button>
{{~else~}}
  {{~#let (element (or @nonInteractiveTageName "span")) as |Tag|~}}
    <Tag ...attributes>{{yield}}</Tag>
  {{~/let~}}
{{~/if}}

(The ~ remove the leading / trailing white space.)

@cah-brian-gantzler
Copy link

Yes, you would have to supply some of the click and default stuff, then the ...attributes would allow for overriding or applying additional things (like class).

I just found this and had not fully understood it so yes, you are right, you would have to solve the innerHTML which is looks like you have some good ideas.

Being a modifier is not the way to go, just agreeing with you.

the ~ is a little known and used handlebars syntax, have know about it for a while

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

No branches or pull requests

3 participants