-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Deprecate component_block
in favour of component
#232
Comments
component_block
in favour of component
component_block
in favour of component
Out of interest, since I believe django-components is still young, would it be disagreeable to use the component name? Again, like JSX: {% Box %}
{% slot "contents" %}
<h1>Inside the box</h1>
{% endslot %}
{% /Box %} Not sure if this makes sense in scope, or consistent with Django usage, but I like the space saving and readability when using a lot of component 😅 |
@EmilStenstrom Interesting idea in the sense of ‘one less tag to be aware of’. On the other hand, it’s less typing if you don’t need any of the slot-related features. I think it’s for this reason that, for example, Vue.js allows both open and self-closing variants of components. Here’s an idea: let’s see how many hits we get for each of the tags in Github’s code search and treat that as an estimate of their relative popularity? @timothyis That sounds a lot like Django Slippers! I think it’s a pretty neat design. What I like less about it is that behind the scenes it leads to a vast proliferation of dynamically created template tags. |
Tried looking at code search just now. Seems other projects use I'm not sure it's possible to have a tag that might, or might not, have an ending tag? How do you know when you should stop parsing? |
I'm not that fond of {% Box %} because it feels too magical. I care a lot about how people that have no idea what django-components is, react to seeing it for the first time. If there's a component in the name, that gives a nice hint that probably they should look in the components dir. Which dir? Well, there's a name there directly after the component name... |
Could use old HTML rules: {% component "Heading1" text="Hello world" /%}
{% component "Heading1" %}
Hello <b>world</b>
{% /component %} Does spoil the ease of a non-block component though. |
Is this really so bad? Much simpler not having to care about the two different tags. {% component "Heading1" text="Hello world" %}{% endcomponent %}
{% component "Heading1" %}
Hello <b>world</b>
{% endcomponent %} |
Did some digging here. Turns out that, out of the 272 hits for the query Notes:
|
Great searching skills, I also have beta code search and got the same results. I guess we miss the cases when component_tags are loaded with a setting, but I think that should be representable still. Conclusion: breaking And I don't see a way to allow component to be optionally closable... |
I mean, it's not terrible. Just a bit more typing than strictly necessary. How's this for a compromise: Yes, we replace 'component_block' by 'component' and drop the current usage of 'component', but we furnish users with an optional shorthand (the JS community loves their shorthands), i.e. @timothyis's idea of adding a A possible argument against it is that @EmilStenstrom, plently of ways to make a component optionally closable, provided the tag contains some reserved token indicating as much. |
To me, it's more important that it's understandable, especially for people seeing the code for the first time. Sure, I think we can add some shortcuts for advanced users, but I think the defaults should be understandable by any Django developer, I think that's a great target to have in mind. |
Given the above, I think... {% component "Heading1" text="Hello world" %}{% endcomponent %}
{% component "Heading1" %}
Hello <b>world</b>
{% endcomponent %} ...would fit the bill nicely. |
I'm still deciding how I feel about this. All in all I agree with the idea of designing for new users, or at least in such as way feels familiar for Django developers. The benefit of going from two tags to a single tag is clear: it's one less thing to learn. By contrast, playing devil's advocate, I can see a few issues with it:
As for a self-closing {% component "button" label="sign up" id="btn-sign-up" level="primary" / %} – I see a few issues:
Nevertheless, I believe users will expect a more compact/shorthand version, since this is something that is supported by (among other)...
|
I wonder if it would be possible to write a component tag that parses the opening tag and, if it finds tags that are not allowed inside a component tag, auto-closes the tag and calls it a day. But if it finds a slot tag, it continues parsing and assumes there must be a end tag coming. Wouldn't that mean that component could be both with and without a closing tag? |
I wonder if {% component "button" label="sign up" id="btn-sign-up" level="primary" end %} Since this follows the Django pattern of using And then this matches the wording/context of {% component "button" %}
<span>Hello</span>
{% endcomponent %} This could also be the other way around (taking GitHub's Primer example), and using a keyword when there is a block (https://github.com/primer/view_components/blob/main/app/components/primer/button_component.html.erb#L2), e.g., {% component "button" with %}
<span>Hello</span>
{% endcomponent %} And then the non-block variant without the |
Also, just an additional thought, but since people are already configuring components in the .py file, you could also ask them to configure whether the component supports being a block or just used without inner content (defaulting to not being a block if no slot is used at all. Maybe something like a setting that allows choosing between using a slot, or using a specific property that's passed into the component. If the prop is passed, don't render the block? Not quite sure on this one, but might thought it might be helpful to throw out there. |
I'd advise against this. I think in some cases it would be possible, but I can also see it failing in subtle ways. Also, parsing would need to be able to backtrack, with extra time complexity as a result. Other arguments against:
Why? Well, after encountering the opening tag, the parser looks for one or more slots/fills. Because none are encountered, the parser doesn't look for a closing tag. One could argue that a dangling closing tag would nevertheless be absorbed, but there is no strict guarantee anymore that the closing tag is associated with the opening tag. |
@timothyis I think you might be onto something. Here's a summary of what we have so far:
@EmilStenstrom, seeing as your original idea was to make the open–close tag pair the base case (i.e. the first thing new users learn), I'm going to guess you're less a fan of |
If only there was djsx 😅 would be the best addition to Django Templates other the django-components. |
I'm not a big fan of making everyone change the component tags to add end of / to them... |
You wouldn't need to with |
@timothyis That's what I'm saying: they would need to go through their project and change every call to component to add a "end", "/" or "with" (whichever we choose). The advantage of finding a way to make the endcomponent tag optional, is that it is fully backwards compatible. We wouldn't even have to remove the component_block tag just yet, so all existing projects could work like they do today. |
To be clear, this is what I'm proposing to be valid ways to use a component (fully backwards compatible):
|
Oof, this turned into quite the essay. ✍️ Apologies. TL;DR I vote component/endcomponent and nothing else. This was already pretty clear, but thanks for the overview. Handy to have an example of everything in one place! I have mixed feelings about this. On the one hand, I can definitely see where you're coming from. On the other, I see a few potential issues. Impressions during usageTo get a feel for the new tag style, I tried it out it in an artifically complex template with embedded components inside fill tags and slot tags, inside components etc., and with mixed closed and unclosed usages. My initial impressions:
Based on the exercise above, my conclusions would be that, yes, we can implement a single component tag; yes, one could get used to reading/writing templates with them, although I'm sorry to see default tags wiped off the table; and finally, crap, we're going to have either open PRs with djhtml and other tools, or learn the hard way that there's a good reason why tags that have dual open and self-closing usages are uncommon in Django – not to mention non-existent in HTML, and sort-of-maybe-possible in XML. Potential issuesAbstractly, there are a few inter-related themes we should pay attention to:
Concretely, in addition to what I observe above, these are my concerns:
On backwards compatibilityThis seems to be a running theme in this thread. Now, normally, I'd absolutely 100% agree with the necessity of backwards compatibility. We know better than to piss off our relatively small user base. However, in light of PR #221, big changes are coming: users are going to have to make substantial changes to their templates anyway. Under the present circumstances, I'd argue it's worth biting the bullet just this once™️ if it means getting this right. What now?Unless someone can point to an example of a widely adopted Django tag that has dual open–void usages, I feel some reluctance about adopting the proposed design. I realize it might sound needlessly limiting to say Django tags must be restricted to one usage only, but I believe not doing so might break the assumptions most users have about how Django tags work. Having said that, this does indeed feel like our most backward compatible design as well as the most (IMO) aesthetically pleasing. My vote would still going to having component be an open tag requiring a closing endcomponent tag and supporting the self-closing/void case by means of a Which leads me to conclude, reluctantly, that the least surprising, most idiomatic, most intercompatible, most future-proof (for e.g. default fills), and potentially more readable option is simply to have component/endcomponent. Full stop. I mean, I personally prefer this...
... over this...
|
Seems like my last comment kind of killed the discussion here. My bad! =p How is everyone feeling about the different proposals? If we can't decide, I'm thinking we should pick two, implement them on separate branches, and see what works best in practice. (I'll take care of the implementing part ;) ) |
Thanks for the extra push! I remember reading the whole block before, but think I might have forgotten about it :) Looking at your example, I still don't like the idea of having everyone having to add {% endcomponent %} tags that doesn't do anything to their projects... at the same time, I agree that the simplest way to do this would be to just go with your suggestion and always use end tags. Thoughts:
|
So I think we agree that the simplest and best option in the long term in going with just the The only open issue left is how to handle backwards compatibility. Suggestion:
What do you think? |
I largely agree. Let's see...
If we make it optional for 'component' tags to be closed, then we're effectively ruling out non-fill tags inside the component–endcomponent span. Whether that's acceptable depends on how we decide to proceed with the default slots over at #231. |
Just an idea: what if we ship a CLI tool exposing useful upgrade commands or something. For instance, migrating users from self-closing component tags to open–closed pairs could be automated with the following regex. (It finds all component tags and replaces them with themselves plus a tacked-on closing tag.) re.sub(
r"(\{%[ ]?component[ ]+(?<!%\}).+?%})",
r"\1{% endcomponent %}",
template
) |
I see three options:
Is 2) the best one? I think so. |
At first glance I agreed with (1) until I realized we'd basically be sending mixed messages. On the one hand, we're saying component_block is on the way out. On the other, "here's this new feature available only on the old component_block." (2) will mean more design work. I would propose having a single CLI entrypoint, e.g. (3) is very doable. Something like My ranking: 3 > 2 > 1. ... Might even be a tie between (3) and (2). |
Agreed that 1) sends mixed messages. So it's out. That leaves 3) and 2). The problem with 3) is that if you miss that there's a new setting, you might not get a grace period at all. You just get the breakage, and no new features until we ship 1.0 instead. That's why I like 2): I breaks things right away, but then gives the user a simple workaround. Design thoughts:
So the final command would be "./manage.py components upgrade close-component-tags ". Thoughts? |
Very nice, I like that command way better. I'd probably tweak it to As for (3), technically, there would zero breakage.
The more I think about it, (2) and (3) don't exclude one another. We could, for instance, implement the new flavor of component tags first, hiding them behind a flag. At the same time, we can start building out the new CLI command to ease the transition. We'll want to iterate on the command a couple of times and test it quite rigorously in order to guarantee it's rock-solid by the time v1 rolls around. |
With breakage for (3) what I mean is that everything will work, until we ship 1.0, and then things break the same way as if we made the change directly. But since you can easily upgrade during that time if we do 2) and 3) at the same time, I agree that that is that best solution! So we have a design idea then? |
I believe we do! 🎉 I'll get working on this soon. Next steps:
|
As for optional ending tag, Tetra somehow solved that: {% load tetra %}
{% @ demo.counter key="counter-1" %}
{% @ demo.counter key="counter-2" current_sum=sum %}
{% @ demo.counter key="counter-3" current_sum=sum / %}
{% /@ demo.counter %}
{% /@ demo.counter %}
Here "@" is the tag (like "component"), and is optionally closed by "/". Maybe habe a look at their code, if you are still keen to implement that. "/@“ is the "endcomponent" aequivalent (which seems a bit unreadable to me there) |
I've been thinking more about this. I think we're making things too hard on ourselves. I still think just having a component tag that requires a endcomponent tag is the best way forward. This is an easy fix, if we ignore backwards compatibility. And since this is pre-1.0 software, I think we can break compatibility. I think it's worth it. So. New plan:
|
Here's a fix for this: #376 |
The only difference between
component
andcomponent_block
is thatcomponent
doesn't require an ending tag. Butcomponent_block
is more popular (I think) and the structures you can build with it are much more complex. In those cases, it would be nicer (and clearer, and require less typing) if you could just use {% component_block %} all the time.Idea: Require
{% component %}
to have an ending tag, and make both tags behave exactly the same. Deprecatecomponent_block
.The text was updated successfully, but these errors were encountered: