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

Fix: Refresh concepts page on jump to #875

Closed

Conversation

Bilelkihal
Copy link
Collaborator

Issues fixed:
#860
#845

Changes

  • Replace reloading the page using Turbo.visit when performing a jump_to, by reloading the whole page (fb06a1a)

Copy link
Collaborator Author

@Bilelkihal Bilelkihal left a comment

Choose a reason for hiding this comment

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

@Bilelkihal Bilelkihal self-assigned this Nov 26, 2024
@Bilelkihal Bilelkihal added the bug Something isn't working label Nov 26, 2024
@Bilelkihal
Copy link
Collaborator Author

@syphax-bouazzouni Deployed to stage you can test it there

Copy link
Collaborator

@syphax-bouazzouni syphax-bouazzouni left a comment

Choose a reason for hiding this comment

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

Even though this is a simple solution for most issues, it is not acceptable because the UX is worse compared to what we had before. For example, see this remark: GitHub Issue #359.

The desired solution for these types of issues is based on the concept of idempotence, which ensures that running the same JavaScript code multiple times does not break the application.

Why your solution works:

  • Turbolinks.visit does not refresh the entire page but only replaces the body. This means the old JavaScript context remains active, and when the new body is rendered, the same JavaScript code is executed again, potentially causing conflicts with the existing code.
  • On the other hand, using window.location fully reloads the page, clearing the old JavaScript context and starting with a fresh state, which resolves the issue.

The proper solution is to fix our JavaScript code to handle idempotency. This means checking if the code has already been initialized before running it again to avoid conflicts.

Here is an example of issues using Turbo.visit in bioportal ncbo#345, fixed like this https://github.com/ncbo/bioportal_web_ui/blob/sync-agroportal/app/javascript/controllers/container_splitter_controller.js

@Bilelkihal Bilelkihal marked this pull request as draft November 28, 2024 13:48
@syphax-bouazzouni
Copy link
Collaborator

Close as the solution was not accepted, @Bilelkihal will open a new PR once working on it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants