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

Set turbolinks to false for the exporter index page #791

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

kirkkwang
Copy link
Contributor

@kirkkwang kirkkwang commented Apr 18, 2023

This commit sets turbolinks to false for exporters because sometimes the form page does not load the correct fields when choosing something from Export From.

In this screenshot, under the Export From should have another field where you search for the collection, but it doesn't...
image

Here is a screenshot of what it should show
image

This commit sets turbolinks to false for exporters because sometimes
the form page does not load the correct fields when choosing something
from `Export From`.
@kirkkwang kirkkwang added patch-ver for release notes bug-fix labels Apr 18, 2023
Copy link
Contributor

@bkiahstroud bkiahstroud left a comment

Choose a reason for hiding this comment

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

Just to clarify, turbolinks will still be enabled on the exporter index page right?

@ShanaLMoore
Copy link
Contributor

ShanaLMoore commented Apr 18, 2023

Just to clarify, turbolinks will still be enabled on the exporter index page right?

We just left an office hour where turbolinks was causing issues with the select box in BL, so Rob recommended we set this to false and contribute it back to bulkrax.

THE ISSUE: When you chose an exporter type, sometimes the follow up input prompt wouldn't display until we reloaded the page (even though they are required).

What in particular are your concerns? cc @bkiahstroud

@bkiahstroud
Copy link
Contributor

@ShanaLMoore I could be wrong, but not having turbolinks might break the jQuery DataTables on the exporter index page.

Though the way I'm reading this code, it's not disabling turbolinks on the exporter index page, it's disabling them from the index page. I.e. when navigating to the form from the index page, it sends a param to disable turbolinks on that page without actually disabling it on the index page. Am I reading that correctly?

Copy link
Contributor

@ShanaLMoore ShanaLMoore left a comment

Choose a reason for hiding this comment

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

cc @kirkkwang Could you confirm and address @bkiahstroud concerns before merging this by making sure the data tables still function correctly?

@bkiahstroud It's my understanding that this should only impact the form elements.

@bkiahstroud
Copy link
Contributor

@bkiahstroud It's my understanding that this should only impact the form elements.

Great, that's completely fine with me. Mostly read "for the exporter index page" in the title and wanted to double check 😅

@kirkkwang
Copy link
Contributor Author

https://share.getcloudapp.com/NQuW5E6y

@bkiahstroud is this the functionality you're talking about? As you can see the turbolinks are false, but i can still sort the table

@bkiahstroud
Copy link
Contributor

@kirkkwang yes, as long as that table is still working as expected than we should be good to go (sort, search, pagination, etc.)

@kirkkwang
Copy link
Contributor Author

Great thanks @bkiahstroud for keeping that in mind! @ShanaLMoore good to merge?

@bkiahstroud
Copy link
Contributor

Thank you both!

@kirkkwang kirkkwang merged commit edf7245 into main Apr 18, 2023
@kirkkwang kirkkwang deleted the disable-turbolinks-for-exporters branch April 18, 2023 23:06
@no-reply no-reply mentioned this pull request Apr 20, 2023
@no-reply
Copy link
Contributor

i'm not sure if it's known, but it looked to me like this issue was introduced in #783 (comment)

it's possible there's a fix available without turning off turbolinks. at least for now, i'm very happy with this, and am confirming that it fixes the issue we've seen on the surfliner side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix patch-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants