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 data-sg-replace functionality #80

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kholdrex
Copy link
Contributor

#62

This PR adds a new data attribute, data-sg-replace. This attribute allows for replacing the browser's history state instead of pushing a new state when navigating.

Changes

  • Modified the visit function to accept a suggestedAction parameter.
  • Added a handler for the data-sg-replace attribute in the HandlerBuilder class.
  • Updated the event listeners to check for data-sg-replace and pass the appropriate action.
  • Added tests for the new data-sg-replace functionality.

Tests

  • Added a new test to ensure visit is called with the replace action when the data-sg-replace attribute is present.
  • Updated existing tests to ensure they continue to work with the additional context of the suggestedAction parameter.

Copy link
Collaborator

@jho406 jho406 left a comment

Choose a reason for hiding this comment

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

Another nice PR! Thank you. I'd like to add some additional context to this functionality. The API that I'm leaning towards for this story look like this:

<a href="/posts" data-sg-visit data-sg-replace>

data-sg-replace sets an extra parameter to the visit function only. Its in the same vein as the existing data-sg-placeholder functionality that look like this:

<a href="/posts" data-sg-visit data-sg-placeholder="/somekey">

My comments below will revolve around that leaning.

superglue/lib/utils/ujs.ts Outdated Show resolved Hide resolved
superglue/lib/utils/ujs.ts Outdated Show resolved Hide resolved
superglue/lib/utils/ujs.ts Outdated Show resolved Hide resolved
superglue/lib/action_creators/requests.ts Outdated Show resolved Hide resolved
@kholdrex kholdrex requested a review from jho406 June 24, 2024 23:29
Copy link
Collaborator

@jho406 jho406 left a comment

Choose a reason for hiding this comment

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

Looking great! Just a few more comments from me.

superglue/lib/action_creators/requests.ts Outdated Show resolved Hide resolved
superglue/lib/action_creators/requests.ts Outdated Show resolved Hide resolved
@jho406
Copy link
Collaborator

jho406 commented Jun 26, 2024

Great work! I'll give this a test during the weekend, and merge.

@jho406
Copy link
Collaborator

jho406 commented Jun 29, 2024

This looks great! I'm marking this for a 1.0 release. I don't want to merge this in quite yet as I want to focus on the typescript conversion, but def on my radar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants