AO3-6392 Comment actions when replying in full page #4381
Conversation
https://otwarchive.atlassian.net/browse/AO3-6392 Reproduce the behavior of add_comment_reply.js
By forcing a non-JS behavior https://otwarchive.atlassian.net/browse/AO3-6392
|
No, I didn't forget to add tests to this PR for over six months, it's just a trick of the light. |
| @@ -657,7 +657,8 @@ def redirect_to_all_comments(commentable, options = {}) | |||
| delete_comment_id: options[:delete_comment_id], | |||
| view_full_work: options[:view_full_work], | |||
| anchor: options[:anchor], | |||
| page: options[:page] | |||
| page: options[:page], | |||
| only_path: true | |||
There was a problem hiding this comment.
Conflicted about this.
On one hand, it does fix every issue I had in tests about being redirect to the wrong domain (example.com).
On the other, changing such a critical piece of code just so tests pass trigger every warning in my brain.
There was a problem hiding this comment.
Yeah, I'm normally not a fan of that either, and the alarm bells are screaming. But ignoring my knee-jerk "let's not do this," I can't think of a reason it shouldn't be only_path: true. We definitely prefer paths to URLs. I think the only reason we hadn't already changed it is we do something in nginx that means this wasn't actively causing issues when we were supporting both HTTP and HTTPS. So this is probably fine.
| @@ -107,7 +107,7 @@ | |||
| <% elsif comment.persisted? %> | |||
| <%= cancel_edit_comment_link(comment) %> | |||
| <% elsif commentable.is_a?(Comment) || commentable.is_a?(CommentDecorator) %> | |||
| <%= cancel_comment_reply_link(commentable) %> | |||
| <%= cancel_comment_reply_link(commentable, remote: !on_js_less_comment_form(commentable)) %> | |||
There was a problem hiding this comment.
It looks like this results in redirecting to the work page (assuming the parent comment is on a work) with the comments section collapsed. That does the job, but I'm not sure if it's the intended behaviour. Thoughts @sarken ?
There was a problem hiding this comment.
Do you mean when replying from the comment section or somewhere else?
I'm looking at the current behavior of the Cancel button on the reply form and what I see is:
- if you're replying to a comment from an email link, e.g.,
http://test.archiveofourown.org/comments/5209801?add_comment_reply_id=5209801, it- redirects to the comment you're replying to without the reply form open if JavaScript is disabled
- closes the reply form without reloading if JavaScript is enabled
- if you're replying to a comment from the comment section on the work, it
- redirects to the work with the comment section closed if JavaScript is disabled
- closes the reply form without reloading if JavaScript is enabled
So if we're talking about from the comment section on the bottom of a work and not a page like the URL I provided, it's not ideal, but it does seem consistent. I guess in an ideal world it would leave the comment section expanded when it redirected to the work, but I'd probably call that out of scope.
There was a problem hiding this comment.
Well...
While attempting to clean up a seemingly unused method, I accidentally found the source of the bug that I had tried to circumvent by forcing a redirection there. So it's not needed anymore as the form now collapses down properly even if the page started in "focused" mode, and everything is smooth in the best of worlds.
Hooray for serendipity?
There was a problem hiding this comment.
Hah, I love when deleting code fixes bugs 😆
I guess it's less relevant now, but the case I was talking about was a special case of 2b: opening the reply link in a new tab (so ending up on a page like http://localhost:3000/chapters/170?add_comment_reply_id=42&show_comments=true#comment_42) and then clicking "Cancel"
There was a problem hiding this comment.
The first case of the if wasn't accessible anymore As the form was hidden if the condition was true
brianjaustin
added
Coder Has Actioned Review
and removed
Reviewed: Ready to Merge
labels
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-6392
Purpose
As per the JIRA, fix extra actions showing up and the Cancel button being "stuck" when answering a comment in full page.
Testing Instructions
Follow the JIRA instructions.
At the end, also try to "Cancel" the reply in the new page.
The page should reload instead of the wrong buttons sticking around.The comment box should now collapse properly, the button "Reply" shows up, and hitting it open the box once again.Credit
Ceithir (he/him)