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

Add a warning about dynamic tags and reword another in Template Directives #6477

Closed
ArmandPhilippot opened this issue Jan 19, 2024 · 4 comments · Fixed by #8585
Closed

Add a warning about dynamic tags and reword another in Template Directives #6477

ArmandPhilippot opened this issue Jan 19, 2024 · 4 comments · Fixed by #8585
Labels
good first issue Good for newcomers help wanted Issues looking for someone to run with them! improve documentation Enhance existing documentation (e.g. add an example, improve description)

Comments

@ArmandPhilippot
Copy link
Contributor

📚 Subject area/topic

Templates directives

📋 Page(s) affected (or suggested, for new content)

https://docs.astro.build/en/reference/directives-reference/

📋 Description of content that is out-of-date or incorrect

I noted two improvements more or less related that can be made on the "Template Directives" page. I could open a pull request but:

  1. I'm not sure what is the best approach to improve the documentation in relation to your style guide,
  2. I'm not enough familiar with the internal mechanism of Astro to be accurate.

define:vars section

The issue

Using define:vars on a <style /> tag in a component which depends on dynamic tags does not work. An issue was reported on the Astro repository but has been closed. It seems a fix is not planned. So I think it would be worth mentioning it in the documentation.

My concerns

There is already a warning about <script /> tags:

define-vars-warning

So I'm not sure what is the best approach to differentiate the two warnings:

  • two warning blocks with an explicit title for each (I never saw this pattern on your documentation),
  • a warning block for <script /> and a note block for <style /> (I saw an example of two different blocks next to each other in the documentation),
  • a single generic "Caution" block with the two warnings next to each other (it can be confusing if the two warnings are not related since the current warning is already multi-lines... I don't know if there is any relation between the the two behaviors),
  • to consider the <style /> tag case as less important and to put it in a new paragraph between the code block and the warning block (there is a similar mention on the "Client directives" section: "Hydration directives are not supported when using dynamic tags".

Proposal

Regardless of the chosen format, we could add the following sentences:

The `define:vars` directive is not supported when using [dynamic tags](https://docs.astro.build/en/core-concepts/astro-syntax/#dynamic-tags). If you cannot wrap the children with an extra element like a `div`, the best approach is to manually add a ``style={`--myVar:${value}`}`` to your `Element`.

is:inline section

Issue

There is a warning block about the directive being applied automatically for any attribute other than src for both <script /> and <style /> tags:

is-inline-warning

But in the next section (define:vars), the warning only mentions the <script /> tag:

Using `define:vars` on a `<script>` tag implies the [`is:inline` directive](https://docs.astro.build/en/reference/directives-reference/#isinline)

There was a previous issue (#3165) about is:inline behavior with define:vars. It seems the statement in is:inline section is wrong: using define:vars on <style /> tag does not automatically apply the is:inline directive.

So I think the warning block in is:inline section should be reworded to reflect the current behavior.

Proposal

Most of the attributes used on a `<script>` or `<style>` tag will imply the `is:inline` directive. The few exceptions are the `src` attribute and, only on the `<style>` tag, the [`define:vars` directive](https://docs.astro.build/en/reference/directives-reference/#definevars).

Perhaps I'm wrong but the lang attribute (ie. <style lang="scss">) does not apply the is:inline directive. Maybe it should be added to the list to prevent confusion.

🖥️ Reproduction in StackBlitz (if reporting incorrect content or code samples)

About the define:vars bug, here is the StackBlitz from the original issue (not mine): https://stackblitz.com/edit/astro-minimal-example-custom-tags?file=README.md

@ArmandPhilippot ArmandPhilippot added the improve documentation Enhance existing documentation (e.g. add an example, improve description) label Jan 19, 2024
@sarah11918
Copy link
Member

Thank you for this thoughtful analysis, @ArmandPhilippot and thank you for your patience as we work through issues ! I can tell you that define:vars is both a blessing and a curse at times. 😄 It is helpful, but has several implications, and lately we have often been recommending alternative methods to achieve the desired outcome instead.

I am also very appreciative of your concern about adding multiple caution blocks together! You're right, that is a pattern we really try to avoid, because usually a smell that content needs reorganizing.

I will call attention to this with some of our devs who can help work through what docs content might need to change, and then depending on what we decide on, we'd be happy to have you make a PR to docs to make the section more helpful!

@ArmandPhilippot
Copy link
Contributor Author

No problem, I understand that it is not a priority. Especially since I thought I saw a proposal to replace define:vars for <script /> tags. I don't know if it is also the case for <style /> tags but it might be worth waiting to see if define:vars is going to be deprecated.

@sarah11918
Copy link
Member

So, I'm getting back to this now (after some time, I know!) and since we still do have define:vars and we're gonna have to live with it, here's what I'd suggest:

Your text addition proposals look great, but yes, we would prefer not to have a bunch of notes and cautions and stuff around.

I'm proposing:

  1. define:vars: Since your first suggestion relates to Dynamic Tags only, let's put the content in that section. There's already a list of bullet points for "When using dynamic tags:" What if we add a third one:
  • The define:vars directive is not supported. If you cannot wrap the children with an extra element (e.g <div>), then you can manually add a style={`--myVar:${value}`} to your Element.
  1. is:inline: I think it is simpler to keep the existing, absolute statement instead of ("most of" which automatically sounds less explicit, no matter what comes after it) I propose keeping the line "The is:inline directive is implied whenever any attribute other than src is used on a <script> or <style> tag." as is, but adding a second line more like:

The one exception is using the define:vars directive on the <style> tag, which does not automatically imply is:inline.

Do you think these proposals would satisfy the issues you've identified? If so, I'd be happy to receive a PR if you'd like to make one yourself! (If not, let me know and I can make the appropriate changes.)

@sarah11918 sarah11918 added good first issue Good for newcomers help wanted Issues looking for someone to run with them! labels Jun 19, 2024
@ArmandPhilippot
Copy link
Contributor Author

Thank you for the feedback and no worries about the elapsed time. I'm fine with your proposals so I opened #8585!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Issues looking for someone to run with them! improve documentation Enhance existing documentation (e.g. add an example, improve description)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants