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

property in anchor element forces a reload of the target #8153

Merged
merged 1 commit into from
Aug 24, 2023
Merged

property in anchor element forces a reload of the target #8153

merged 1 commit into from
Aug 24, 2023

Conversation

martrapp
Copy link
Member

@martrapp martrapp commented Aug 18, 2023

Changes

In an anchor element, the data-astro-reload property (with no value or with a value other than 'false') forces a reload of the target document (instead of a view transition).

Closes #7947

Testing

manually tested on the example given in #7947, outcome documented there.
[update 2023-08-22]: added e2e test

Docs

The documentation of the (still experimental) View Transitions needs to be updated.

Not sure if the data property will be the final interface for users or if there will be syntactic sugar (e.g. transition:reload).
I plan to add one or more properties in the next few days to address other issues, in particular a property to control cross-page scrolling behavior (as opposed to on-page scrolling behavior)

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Aug 18, 2023

🦋 Changeset detected

Latest commit: 52606a3

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 18, 2023
@matthewp
Copy link
Contributor

Want to bikeshed on the name a little bit. astro-reload sounds a little off to me because we're not reloading the page; the page has never been loaded in the first place. But if this is a common name other similar implementations use then I think it's ok.

Just did a check:

Could not find other examples.

@martrapp
Copy link
Member Author

force-load?
mpa-load?

@ematipico
Copy link
Member

Maybe astro-load or astro-server-load? Or astro-force-load like suggested. "mpa" uses an acronym that is not well known.

@matthewp
Copy link
Contributor

matthewp commented Aug 21, 2023

cc @delucis and @bluwy our naming experts.

@delucis
Copy link
Member

delucis commented Aug 21, 2023

Given our current directives related to this are all transition:*, I’d imagine something like transition:none would make most sense here?

Although… would clicking a link with this property override any transition:persist and blow away that DOM? If so, that makes the transition:* namespace a much less good fit.

Do we have a description of use cases somewhere? Those might help with naming.

@matthewp
Copy link
Contributor

@delucis This just means you want to opt-out of client-side routing for a particular link. One use-case is that the link goes to a file rather than another page, for ex. <a href="/chart.pdf">Chart</a>.

Note that directives are used for server-side processing in some way; transition:animate creates a stylesheet. There's no server-side stuff happening in this one. The attribute will remain in the output HTML. Doesn't mean we can't use a directive like name, but something to consider.

@bluwy
Copy link
Member

bluwy commented Aug 22, 2023

SvelteKit uses data-sveltekit-reload so there's an initial pattern. Though I'd say it's confusing at times too, it's not really a reload, but letting the browser do its thing instead. Maybe data-astro-no-load, data-astro-skip-route, data-astro-skip-navigate, data-astro-skip, data-astro-ignore? I'm not really sure what's the best name here 😄

Could also think ahead how this plays if we want to make prefetch first-class in the future

@delucis
Copy link
Member

delucis commented Aug 22, 2023

Yeah, if we’re reasonably happy with how astrojs/prefetch works currently, that could maybe help us pick a name that works well with those? Although that’s opt-in at the moment via rel="prefetch" and rel="prefetch-intent", so perhaps doesn’t help.

@matthewp
Copy link
Contributor

What about data-astro-no-transition? Keep it specific to what happens otherwise?

@martrapp martrapp marked this pull request as draft August 22, 2023 18:39
@martrapp martrapp marked this pull request as ready for review August 22, 2023 18:56
@matthewp
Copy link
Contributor

I think I'm coming around to just using data-astro-reload to keep it consistent with svelte. I don't love it, but don't love the alternatives.

@martrapp martrapp marked this pull request as draft August 23, 2023 06:54
@martrapp
Copy link
Member Author

There are only two hard things in Computer Science: cache invalidation and naming things.

My 2ct

I won’t use “transition” because technically it is not mainly about visual transition but whether or not the document is updated with new content (swap) or replaced by a new document. There could also be swap without transition.

link would sound good to me but none of the defined values (https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel) seem to fit our purpose

another try:
data-astro-fresh-document
data-astro-browser-default-navigation
data-astro-browser-default
data-astro-browser-navigation
sata-astro-default-navigation

These would also express that we leave it to to browser to choose viewers based on content-type

@martrapp martrapp marked this pull request as ready for review August 23, 2023 09:53
@@ -276,6 +276,7 @@ const { fallback = 'animate' } = Astro.props as Props;
if (
link &&
link instanceof HTMLAnchorElement &&
(link.dataset.astroReload ?? 'false') === 'false' &&
Copy link
Contributor

@matthewp matthewp Aug 23, 2023

Choose a reason for hiding this comment

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

is there a reason why someone would use a false value here? If not I think we can just check for its existence to save a few bytes.

Copy link
Member Author

Choose a reason for hiding this comment

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

For me it looks odd if someone writes: reload="false" and gets a reload.
But at the end it is a matter of documentation: "Any value assigned to this attribute is ignored."
10 Bytes saved :-) !

@matthewp
Copy link
Contributor

I think given the lack of great options we should just stick with data-astro-reload and think about it more later.

@martrapp martrapp marked this pull request as draft August 23, 2023 14:40
@martrapp martrapp marked this pull request as ready for review August 23, 2023 14:41
@matthewp matthewp changed the base branch from main to next August 23, 2023 15:01
@matthewp
Copy link
Contributor

I'm changing the base of this to next as we are not going to do another minor release of 2.x, so this will go out with 3.0.

I'm going to document this as part of withastro/docs#4320

@matthewp
Copy link
Contributor

@martrapp would you mind resolving this conflict? I would do it but it's part of the code that you most recently updated. Once that's done this is ready to be merged. Thanks!

* swap attributes of the root element

* + changeset
@martrapp martrapp marked this pull request as draft August 24, 2023 13:32
@ematipico ematipico merged commit 52606a3 into withastro:next Aug 24, 2023
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDF attachment not rendering right when ViewTransition is enabled.
5 participants