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

x-for / template tag is the first item issue | Adam Wathan mentioned on his video #387

Closed
MuzafferDede opened this issue Apr 17, 2020 · 17 comments

Comments

@MuzafferDede
Copy link
Contributor

MuzafferDede commented Apr 17, 2020

There is an issue with the template tag being the first child element of the loop. @adamwathan has mentioned here:
https://youtu.be/v_1casra9g4?t=2883

update: Video starts at the moment he mention the issue. You don't need to watch entire video. Also he didn't talk about any fix, just self noted to talk to Caleb about it.

here is the sample code:
https://codepen.io/SimoTod/pen/WNQwzrr thx @SimoTod

Is this a known issue?

@HugoDF
Copy link
Contributor

HugoDF commented Apr 17, 2020

Not sure I understand the issue

@SimoTod
Copy link
Collaborator

SimoTod commented Apr 17, 2020

It's a good observation but I think it's the only possible way at the moment.
Vue created a Virtual DOM so it can keep all the noise separated from the page.
Alpine works directly on the Dom so the template tag needs to be there and it can't be removed otherwise it would brake when the reactivity kicks in.

Your stylesheets needs to take it into account unless something changes in the future.

I haven't watched the video coz I don't have time now so sorry if I suggest something that has already been tried and discarded but why not excluding template tags from that selector?
.divide-pink-800 > :not(template) + * cc @adamwathan

It won't work in cases where you do want the border even when the previous element is a template (if there are any) but not sure if it's a concern.

P.s. @MuzafferDede your fiddle does not show the issue because you didn't save the css part.
Here's a pen to show the issue and a possible(?) css solution: https://codepen.io/SimoTod/pen/WNQwzrr

@MuzafferDede
Copy link
Contributor Author

MuzafferDede commented Apr 17, 2020

@HugoDF Sorry, I didn't describe the details because i thought it was a known issue from last live streaming. If i am not remembering wrong, Caleb was discussing about the new child element should be inserted after or before.

@SimoTod Don't worry. Adam didn't try any fix, he just switch to use Vue for his case. :)

Yeah, i forgot the css. But you got the issue and actually provided a considerable hack in my opinion. I am not sure if @adamwathan will consider this hack for alpine just because of alpine's implementation. But this will not change the fact that templates are being first child of the parent element will cause issues in many other scenarios.

i was looking at x-for's this piece of code

// for.js
function lookAheadForMatchingKeyedElementAndMoveItIfFound(nextEl, currentKey) {
    // If the the key's DO match, no need to look ahead.
    if (nextEl.__x_for_key === currentKey) return nextEl

    // If the don't, we'll look ahead for a match.
    // If we find it, we'll move it to the current position in the loop.
    let tmpNextEl = nextEl

    while(tmpNextEl) {
        if (tmpNextEl.__x_for_key === currentKey) {
            return tmpNextEl.parentElement.insertBefore(tmpNextEl, nextEl)
        }

        tmpNextEl = (tmpNextEl.nextElementSibling && tmpNextEl.nextElementSibling.__x_for_key !== undefined) ? tmpNextEl.nextElementSibling : false
    }
}
----
function addElementInLoopAfterCurrentEl(templateEl, currentEl) {
    let clone = document.importNode(templateEl.content, true  )

    if (clone.childElementCount !== 1) console.warn('Alpine: <template> tag with [x-for] encountered with multiple element roots. Make sure <template> only has a single child node.')

    currentEl.parentElement.insertBefore(clone, currentEl.nextElementSibling)

    return currentEl.nextElementSibling
}

where the question come out.

if there was a way to keep template element being the last child of the loop might be an alternative way. As In while loop, inserting the new elements before template but after the last new child.

Or alternatively if it's possible to keep templates another place in DOM and make a parent - child relation for reactivity.

@tlgreg
Copy link
Contributor

tlgreg commented Apr 17, 2020

Usually, it messes things up where you would use odd/even/nth-child type of rules in css. These can't be fixed with :not(template), instead you can toggle the styles themselves with alpine.

As for the implementation of x-for. Either the current way or I could imagine using a wrapper element for the iterated elements, but that would also mean that there can't be anything else on the same level as the iterated elements.
Something like:

<template x-for="item in items" x-el="ul" class="item-list">
  <li x-text="item.text" x-key="item.id" />
</template>
<p>...</p>

And it would add it as the next element.

<template x-for="item in items" x-el="ul" class="item-list">
  <li x-text="item.text" x-key="item.id" />
</template>
<ul class="item-list">
  <li>foo</li>
  <li>bar</li>
</ul>
<p>...</p>

@SimoTod
Copy link
Collaborator

SimoTod commented Apr 17, 2020

Fair points. The wrap attribute could maybe be optional (at least in v2 so we don't break stuff) but it sounds a viable solution. It would mimic what vue does which is one of the goals of the library.

However i won't classify it as a bug but an interoperability issue (which i believe Caleb is keen on fixing any way given that Tailwind and Alpine belong to an ecosystem of tools used by the same people).

@calebporzio
Copy link
Collaborator

I paired with Adam on some potential solutions yesterday.

The problem with a explicit :not template selector for Tailwind core is the added specificity.

Adam may adjust Tailwinds core to allow for internal specificity balancing which could solve our problem.

I'm not keen on changing the x-for implementation just for a tailwind class.

Id sooner advise a css override, but maybe we won't even have to worry about this.

Also, Adam likes Alpine and is willing to work with us.

@tlgreg
Copy link
Contributor

tlgreg commented Apr 17, 2020

This is not tailwindcss related though, even if the opening post focused on it. This is generally css related issue where the selector tries to target elements based on its markup position.

I'd say with tailwindcss it is actually a smaller issue because you would mostly use utility classes with it, making it easy to conditionally apply those with alpine.

Some other selectors where an extra element will yield unexpected results:

/* would actually be even */
.iterated-item:nth-child(odd) {}
/* would never match */
.iterated-item:first-child {}

In some cases :first-of-type helps where :first-child fails (but not always an option) and knowing it's an alpine loop you can change the :nth-child calculation taking into account the extra first element.

This does mean that the css has to take this into account and makes it harder to use with 3rd-party libraries.

@MuzafferDede
Copy link
Contributor Author

This is not tailwindcss related though, even if the opening post focused on it. This is generally css related issue where the selector tries to target elements based on its markup position.
This does mean that the css has to take this into account and makes it harder to use with 3rd-party libraries.

I agreed with these and actually for these reasons i open the issue. Yes, neither alpine or tailwindcss should add specificity for each other. Focus has to be on core css selectors which they should not be interrupted.

It could be a milestone for future x-for core behavior.

@HugoDF
Copy link
Contributor

HugoDF commented Apr 17, 2020

Would Alpine inserting the templated output before the "template" tag help?

It would essentially shift the issue to last-child instead of first-child

@MuzafferDede
Copy link
Contributor Author

Would Alpine inserting the templated output before the "template" tag help?

It would essentially shift the issue to last-child instead of first-child

I believe that will cover the most of the use cases but still there will be last-child issues. Which is as Adam mentioned in real-world scenarios wont be much.

Adam has took care of Tailwindcss part by including in the :not(template) selector in the Tailwindcss. But other folks who doesn't use Tailwindcss will be out of luck whenever they target the first child element with css.

I'm not keen on changing the x-for implementation just for a tailwind class.

@calebporzio i hope we are on the same page that this issue is not related to Tailwindcss, it's a general issue. Any 3rd-party libraries which targets the first child element will have this issue.

@HugoDF
Copy link
Contributor

HugoDF commented Apr 17, 2020

@MuzafferDede makes sense, thanks for getting back to me

@HugoDF
Copy link
Contributor

HugoDF commented May 9, 2020

Does this need to still be a separate issue to #450?

Seems the Tailwind stuff has been dealt with (on Tailwind side), so now this issue and that one are seem to be coming at "why can't we remove template?" from different angles.

@MuzafferDede
Copy link
Contributor Author

I am not sure if there is any issue maps that template element being first child of the loop which cause css select issue. But yeah, many of the open issues will be closed if "template" tag is removed or being first child of the loop. Your call @HugoDF

P.S.
It is good move to close all open issues that's duplicated or similar to others. We should narrow the focus on issues to reference and track them.

@HugoDF
Copy link
Contributor

HugoDF commented May 9, 2020

Cool thanks, closing this in favour of #450 since Adam has pushed a fix out for the Tailwind issue on the Tailwind side.

@HugoDF HugoDF closed this as completed May 9, 2020
@craigerskine
Copy link

The problem I've run into with this model is accessibility. I often use semantic code like

<ul role="tablist">
  <template x-for="...">
  <li><a href="#" role="tab">...</a></li>
  </template>
</ul>

When rendered the template tag is still in the DOM and accessibility checkers trigger a fail because a <template> tag cannot be a direct child of a <ul> tag. I end up having to remove the proper semantic code and use aria roles on generic <div> tags 😭. This is the biggest downfall of alpine IMO.

@adamwathan
Copy link
Contributor

adamwathan commented Feb 9, 2023

I think whatever tool you're using might have a false positive there — <template> tags are totally valid children of <ol> and <ul> elements according to the HTML spec:

https://html.spec.whatwg.org/multipage/grouping-content.html#the-ul-element

image

image

@craigerskine
Copy link

I think whatever tool you're using might have a false positive there — <template> tags are totally valid children of <ol> and <ul> elements according to the HTML spec:

Thanks, @adamwathan. I truly appreciate the help.

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

7 participants