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

Remove the vote-table modal if it exists, on vote, to force a refresh #2045

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

nimmolo
Copy link
Contributor

@nimmolo nimmolo commented Mar 19, 2024


Updating the table in the modal

The table wasn't refreshing because we're using modal_link_to, which re-opens a modal if it exists. This behavior is helpful and expected in the case of a form in progress, but here it's preventing reload of the modal content.

Just needed to add something to the Stimulus naming-vote_controller so that whenever the user casts a vote, we delete the modal_naming_votes_#{naming.id} modal if it's in the DOM.

To test:

  • Open the vote table modal, close it
  • Change your vote on a naming
  • Open the vote table again, the tally should be updated

Refreshing the page more simply

An update to the Namings table is a case of a Turbo partial page refresh: the namings table should be updated, as well as possibly the title of the obs, and the name info panel, but nothing else on the page should change, and the page should not "jump" back to the top - it should retain scroll position.

Before Turbo 8 the DOM elements had to be explicitly changed in a turbo_stream template.

Turbo 8 allows us to simply redirect to the same action for a refresh. Turbo detects that it's the same page, loads the HTML of the page without reloading Rails, detects the differences in DOM elements (having IDs) with a diffing algorithm, and "morphs" changed DOM elements into the page, all without a page refresh.

This means we don't need to write a special turbo_stream updating template, and in some cases we don't even need any separate Turbo response. We can just redirect to the page, as we used to do in Rails 4.

To maintain scroll position, i had to add a meta tag to the head element.

To test:

  • Downvote an existing naming
  • Propose a new naming and give it a high vote so it becomes the new "consensus" name
  • The naming table, title, name panel and flash should all refresh

@nimmolo nimmolo requested a review from JoeCohen March 19, 2024 21:56
@nimmolo nimmolo marked this pull request as ready for review March 19, 2024 21:56
@nimmolo nimmolo requested a review from mo-nathan March 20, 2024 00:02
JoeCohen
JoeCohen previously approved these changes Mar 20, 2024
Copy link
Member

@JoeCohen JoeCohen left a comment

Choose a reason for hiding this comment

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

I did both manual tests and it works fine.
Looked at the changed files, but am still way behind on understanding js etc., so I have nothing to add.

@JoeCohen JoeCohen self-requested a review March 20, 2024 13:47
Copy link
Member

@JoeCohen JoeCohen left a comment

Choose a reason for hiding this comment

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

The tests consistently fail locally with this failure, which seems relevant to the fixes in this PR:

joe@Josephs-iMac mushroom-observer % rails test:all
...
[Screenshot Image]: /Users/joe/mushroom-observer/tmp/capybara/failures_test_identify_index_naming_and_vote_ui.png
 FAIL HelpIdentifySystemTest#test_identify_index_naming_and_vote_ui (167.08s)
        expected to find css "#modal_obs_247545329_naming" but there were no matches
        test/system/help_identify_system_test.rb:30:in `test_identify_index_naming_and_vote_ui'

...

Fails even when run on its own.

% rails t test/system/help_identify_system_test.rb:30
Started with run options --seed 25344

[Screenshot Image]: /Users/joe/mushroom-observer/tmp/capybara/failures_test_identify_index_naming_and_vote_ui.png
 FAIL HelpIdentifySystemTest#test_identify_index_naming_and_vote_ui (15.50s)
        expected to find css "#modal_obs_247545329_naming" but there were no matches
        test/system/help_identify_system_test.rb:30:in `test_identify_index_naming_and_vote_ui'

@JoeCohen JoeCohen dismissed their stale review March 20, 2024 13:57

Test failure noted in comments

@coveralls
Copy link
Collaborator

coveralls commented Mar 21, 2024

Coverage Status

coverage: 94.417% (-0.01%) from 94.428%
when pulling 05127a5 on nimmo-refresh-votes-table-on-vote
into ada80ef on main.

@nimmolo nimmolo merged commit cc7deaa into main Mar 21, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confidence vote table is not updating on vote
3 participants