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 script behavior section #6798

Merged
merged 14 commits into from
Mar 9, 2024
Merged

update script behavior section #6798

merged 14 commits into from
Mar 9, 2024

Conversation

OliverSpeir
Copy link
Contributor

@OliverSpeir OliverSpeir commented Feb 6, 2024

Description (required)

Reworks the Scripts section of view-transitions.mdx, relativey significantly. Adds documentation for new attribute of <script> tags data-astro-rerun

Related PR

withastro/astro#9977

For Astro version: 4.5

Would love your review on this @martrapp

Copy link

vercel bot commented Feb 6, 2024

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

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Mar 9, 2024 7:02pm

@martrapp
Copy link
Member

martrapp commented Feb 6, 2024

As for the broken link: It is mdx. You could replace the ## with an <h2> and the old id or insert an anchor with the old id right in front of the ##, see suggestion.

Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@OliverSpeir
Copy link
Contributor Author

As for the broken link: It is mdx. You could replace the ## with an <h2> and the old id or insert an anchor with the old id right in front of the ##, see suggestion.

Thank you that's brilliant

Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@martrapp
Copy link
Member

martrapp commented Feb 6, 2024

Sorry for the nitpicking. The main reason for the suggested changes are scripts like this:

<script type="module" data-astro-reload>...</script>

which are module scripts (= deferred loading) but not bundled (instead inlined) and can be re-executed without problems.

So much for: "scripts are run in sequential order"

<script type="module">
  console.log("i log later");
</script>

<script is:inline>
  console.log("i log first");
</script>

(both are inline even though it is only explicit for the second one)

OliverSpeir and others added 3 commits February 6, 2024 16:04
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@OliverSpeir
Copy link
Contributor Author

I appreciate the nitpick especially when its as easy as hitting commit suggestions haha and I really do want this section of the docs to be as clear as possible because this is a huge part of the view tranistions experience

haha at least running a module will run when its meant to (deffered) but yeah the whole any attribute creates is:inline is weird

@sarah11918 sarah11918 added the improve documentation Enhance existing documentation (e.g. add an example, improve description) label Feb 8, 2024
@OliverSpeir OliverSpeir 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 Feb 18, 2024
@OliverSpeir
Copy link
Contributor Author

@sarah11918 Could you take another look at this when you get a chance?

@sarah11918
Copy link
Member

Yup! These 4.5 docs are on deck for this week! 🙌

@sarah11918 sarah11918 added this to the 4.5.0 milestone Feb 20, 2024
Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Thanks @OliverSpeir, just a few nits suggestions, after this it should be good to go 🙌

src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
OliverSpeir and others added 2 commits March 7, 2024 14:38
Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
@OliverSpeir
Copy link
Contributor Author

Thank you! Great catch

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.

OK! Thanks for waiting patiently @OliverSpeir ! I went through this and think some restructuring of the content (both the original and yours) makes it even more helpful, so see whether my attempt makes sense here, and let me know what you think!

src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
@sarah11918 sarah11918 changed the base branch from main to 4.5.0 March 8, 2024 14:13
@sarah11918 sarah11918 added the minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! label Mar 8, 2024
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@OliverSpeir
Copy link
Contributor Author

@sarah11918 brilliant suggestions thank you

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.

Thanks, @OliverSpeir ! I really appreciate you adding the extra context over and above documenting astro-data-rerun!

@sarah11918 sarah11918 merged commit 9235649 into withastro:4.5.0 Mar 9, 2024
3 checks passed
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. improve documentation Enhance existing documentation (e.g. add an example, improve description) merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants