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

Implement checkbox submit without jquery #2161

Merged
merged 1 commit into from
Dec 3, 2021
Merged

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Oct 3, 2019

No description provided.

console.warn("do_bookmark_toggle_behavior is deprecated. Use doBookmarkToggleBehavior instead.");
return Blacklight.do_bookmark_toggle_behavior();
}
}); //change form submit toggle to checkbox
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this is the generated (dist) code, jump to app/javascript/* for source changes.

@jcoyne jcoyne mentioned this pull request Sep 2, 2020
@cbeer cbeer moved this from Backlog to Review required in Blacklight Summit Triage Board Nov 9, 2021
@jcoyne jcoyne force-pushed the checkbox-without-jquery branch 2 times, most recently from 0c76d3a to 738fa7b Compare November 18, 2021 20:39
@@ -11,7 +10,6 @@ export default {
Autocomplete,
BookmarkToggle,
ButtonFocus,
CheckboxSubmit,
Copy link
Member

Choose a reason for hiding this comment

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

There still is a checkbox_submit.js with an export, why do these two lines get removed from index.js, is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is imported by bookmark_toggle.js, which is the only place it is used.

Copy link
Member

@jrochkind jrochkind Dec 2, 2021

Choose a reason for hiding this comment

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

Makes sense, thanks! Is this possibly a backwards compat change, if a local app was using it? If so, that's maybe okay since we're shooting for major version? Just wanted to be clear (and is there anywhere we're listing backwards compat changes?)

Copy link
Member Author

Choose a reason for hiding this comment

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

That was never available in a release. That file was just created 2 weeks back: 4976e2d

@jrochkind
Copy link
Member

Can anyone confirm that this behavior is tested by some automated test that's running? If so, I feel more comfortable approving without manually testing myself or having the capacity to code-read it more cursorily.

@jcoyne
Copy link
Member Author

jcoyne commented Dec 2, 2021

@jrochkind yes, it's tested via spec/features/search_context_spec.rb

@jcoyne jcoyne merged commit a5d92d2 into main Dec 3, 2021
@jcoyne jcoyne deleted the checkbox-without-jquery branch December 3, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants