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

Update alpinejs.mdx #6534

Merged
merged 10 commits into from
Jan 26, 2024
Merged

Update alpinejs.mdx #6534

merged 10 commits into from
Jan 26, 2024

Conversation

florian-lefebvre
Copy link
Member

Description (required)

Docs for withastro/astro#9751, mostly copied from the vue docs

Related issues & labels (optional)

  • Suggested label: minor-release

For @astrojs/alpinejs version: 0.4.0. See astro PR #9751.

Copy link

vercel bot commented Jan 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Jan 26, 2024 2:16pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs-i18n ⬜️ Ignored (Inspect) Jan 26, 2024 2:16pm

@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) labels Jan 22, 2024
Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny suggestion @florian-lefebvre, awesome job 😁

src/content/docs/en/guides/integrations-guide/alpinejs.mdx Outdated Show resolved Hide resolved
Co-authored-by: Paul Valladares <85648028+dreyfus92@users.noreply.github.com>
Copy link
Contributor

@hippotastic hippotastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this docs addition! As you added this new feature to address common Alpine developer issues with extending Alpine as we discussed on Discord, it might be useful to include an example that shows how to add a popular Alpine plugin.

In the Discord thread, developers using Alpine mentioned @alpinejs/intersect and @alpinejs/persist, which might serve as an example.

Also, developers mentioned that they tried installing a plugin by adding <script defer src="https://unpkg.com/@alpinejs/intersect@3.x.x/dist/cdn.min.js"></script>, which seems to be a common method of installing Alpine plugins on other sites. It would be great to validate if this method works reliably with your new feature, and if not, document this and recommend the alternative instead (installing the package as a dependency, importing it into the entrypoint file and calling e.g. Alpine.plugin(persist) for the persist plugin).

@florian-lefebvre
Copy link
Member Author

I have updated the entrypoint example to use the the intersect plugin as a npm package. As for the the script import, I have no idea if it works. I'm first going to test it


The Alpine.js integration does not give you control over how the script is loaded or initialized. If you require this control, consider [installing and using Alpine.js manually](https://alpinejs.dev/essentials/installation). Astro supports all officially documented Alpine.js manual setup instructions, using `<script>` tags inside of an Astro component.
```diff lang="html"
<head>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being so insistent on this, but I have to ask: Were you really able to successfully test this? :) From what I've read, this was one of the issues that users were struggling with in the Discord support thread: the Alpine plugins loaded via deferred script tags were not guaranteed to have finished loading before the Alpine integration would initialize Alpine, and were therefore not reliably picked up by Alpine.

Did you change anything in the code that makes this possible now?

If it's possible now, awesome! If so, I'd love to see where this <head> tag should be placed by adding more context, e.g. a file name tab and the surrounding code - users should be able to know where they should add this so that the plugin is available on all pages of their site.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure no problem! Have a look at this e2e test withastro/astro#9812, I think that's exactly the scenario you're mentioning. It seems to work without my recent changes

Copy link
Contributor

@hippotastic hippotastic Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's awesome that you have added an E2E test! However, is this test actually effective in checking if the collapse plugin is working? From what I see, it's checking whether <p id="foo" x-show="expanded" x-collapse> has a class of hidden, and if not, it assumes the test has passed. But is this not the case even when the plugin is not loaded at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually checked and yes, the collapse plugin adds a hidden attribute so that's a safe check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you for confirming. I also checked it manually now, even while simulating a slow connection using devtools, and the plugin always worked. I wonder why it didn't for the users reporting issues on Discord.

So, the only thing that's left is providing a code block file name and a little bit of context in your example so that users reading it will know immediately where to put this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me if that looks good enough to you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot better, thank you! I just left a comment regarding file naming (layout vs. page). It was actually already helpful that we talked about this because I didn't even have a layout in mind, but that's actually a great way to add the script to each page head!

Co-authored-by: Hippo <6137925+hippotastic@users.noreply.github.com>
Copy link
Contributor

@hippotastic hippotastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such an amazing job @florian-lefebvre, thank you as well @hippotastic for deep diving into this, LGTM 😁

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Florian! I took an editing pass and mostly: moved config up before usage, since you will likely configure before using; Expressive Code'd the code sample, and just tightened up some wording! Since the feature is released, I'll merge this now, and you are welcome to PR changes/corrections if I messed it up too badly. ;)

@sarah11918 sarah11918 merged commit 7d5980f into main Jan 26, 2024
7 checks passed
@sarah11918 sarah11918 deleted the florian-lefebvre-alpinejs-entrypoint branch January 26, 2024 14:25
dreyfus92 added a commit that referenced this pull request Jan 31, 2024
* Update add-content-collections.mdx

Just for Lunaria

* Update 2.mdx

Update with PR #6558

* Update 1.mdx

Update with PR #6571

* Just for lunaria Update 3.mdx

Update with PR 6544 for lunaria

* Update error-reference.mdx

With PR #6623

* Update astro-glob-no-match.mdx

Update with PR #6623

* Create i18n-not-enabled.mdx

* Add i18n-not-enabled.mdx

With PR #6623

* Create missing-index-for-internationalization.mdx

* Add missing-index-for-internationalization.mdx

With PR #6623

* Update manual.mdx

With PR #6653

* Update prefetch.mdx

Update with PR #6573

* Update tailwind.mdx

Update with PR #6662

* Update alpinejs.mdx

Update with #6534

* Update src/content/docs/fr/install/manual.mdx

Co-authored-by: Thomas Bonnet <thomasbnt@protonmail.com>

* Update src/content/docs/fr/guides/integrations-guide/alpinejs.mdx

Co-authored-by: Thomas Bonnet <thomasbnt@protonmail.com>

* Update src/content/docs/fr/reference/errors/missing-index-for-internationalization.mdx

Co-authored-by: voxel!() <voxelmc@hotmail.com>

* Update src/content/docs/fr/reference/errors/i18n-not-enabled.mdx

Co-authored-by: voxel!() <voxelmc@hotmail.com>

* Update src/content/docs/fr/reference/errors/astro-glob-no-match.mdx

Co-authored-by: voxel!() <voxelmc@hotmail.com>

---------

Co-authored-by: Thomas Bonnet <thomasbnt@protonmail.com>
Co-authored-by: voxel!() <voxelmc@hotmail.com>
Co-authored-by: Paul Valladares <85648028+dreyfus92@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants