-
Notifications
You must be signed in to change notification settings - Fork 146
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
Link to textanswer_edit
with a button instead of in the answer text
#1689
Conversation
evap/staff/templates/quick-review.js
Outdated
@@ -164,6 +168,7 @@ $(document).ready(() => { | |||
type: "POST", | |||
url: "{% url 'staff:evaluation_textanswers_update_publish' %}", | |||
data: parameters, | |||
success: function(data) { if(action == "textanswer_link" && data) window.location = data; }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit hacky, I don't know what we concluded the last time we talked about it. We now thought that it could possibly be clearer if the button would be in a form by default (which would submit and redirect by default) and we only cancel the submission when we want to do ajax instead.
Even better: Always submit forms and use standard conventions to ignore the response. According to https://stackoverflow.com/a/3283126/13679671 (I also tested with the evaluation_edit view) the browser will not reload the page if the status code is 204 - that means you could just return that for all cases except for the one where you really want to redirect. This means a small refactor is needed about the way these buttons work - I don't know how keen you are on HTML forms, but it probably shouldn't be too bad.
If you don't want to spend more time on this, you can also leave it like this for now and we will fix it later :)
Co-authored-by: Johannes Wolf <johannes-wolf@posteo.de>
Co-authored-by: Johannes Wolf <johannes-wolf@posteo.de>
Co-authored-by: Johannes Wolf <johannes-wolf@posteo.de>
Two things functionality-wise:
Not sure how much weight should be put on those things. I would prefer if both were fixed, but I leave the final call to @janno42 and others :) |
There is a third thing too. If you open the edit window from full review and save you will be redirected to the quick review. All those three things have been there before my PR. If you like I would fix them. But this is a very rarely used feature, if I understood correctly 10x a year and the bugs are not critical. I think it is not worth the time. You could open a issue with very low priority? |
The two things I wrote are old? How do they occur? Aren't these problems that are specific to the new feature? It would of course be nice to have all of it fixed, but we can probably first merge this PR for the functionality and make a second issue that makes-it-right ™️, this one shouldn't be put back for too long though |
I did not change the links, just where they are located. |
Co-authored-by: Johannes Wolf <johannes-wolf@posteo.de>
Co-authored-by: Johannes Wolf <johannes-wolf@posteo.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ Meets requirements
✔️ UI functionality checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thought from me, other than that this looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current code is okay for now, @niklasmohrin will create a follow up issue where we'll try to refactor this to use less javascript and instead utilize html forms and behavior of browsers on different http status codes.
textanswer_edit
with a button instead of in the answer text
No description provided.