"I can't give feedback" button clears text answers #2469
Conversation
d60e52a to
37005d0
Compare
niklasmohrin
left a comment
There was a problem hiding this comment.
Haven't tested yet; code structure looks good, some comments on implementation
janno42
left a comment
There was a problem hiding this comment.
Nice! Please collapse all textboxes from the additional textanswers that have been cleared. When a user opens a collapsed contributor again, these textboxes should be collapsed, as they were initially.
richardebeling
left a comment
There was a problem hiding this comment.
code looks generally good to me
@niklasmohrin can you take a look at the modal logic? I think it matches what we did for the reset evaluation button.
niklasmohrin
left a comment
There was a problem hiding this comment.
Nice work, there seem to be a handful of moving parts, but complexity is still manageable in my opinion.
Please make sure to actively review the code in the Files Changed tab of github to ensure everything is clean before requesting reviews, right now there is for example console.log, unrelated formatting, or unnecessarily moved lines like at the // Disable this button, until user changes a value line. Otherwise we reviewers might overlook the important bits and the PR could take longer to merge overall.
I think this PR is looking very promising! I hope we can manage to capture the changes in tests so that we reduce the risk that someone messes this up on accident at a later time - it's super hard to always remember all the small UI flows when reviewing, so having tests as a safety net is a great help.
| const radioButtons = voteArea.querySelectorAll(".vote-inputs [type=radio]:not([value='6'])"); | ||
| for (const radioButton of radioButtons) { | ||
| if (radioButton.checked) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
I think we can do something like this:
| const radioButtons = voteArea.querySelectorAll(".vote-inputs [type=radio]:not([value='6'])"); | |
| for (const radioButton of radioButtons) { | |
| if (radioButton.checked) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| return voteArea.querySelector(".vote-inputs [type=radio]:not([value='6']):checked") !== null; |
There was a problem hiding this comment.
we cannot, because the checked in my code is not a css property, but instead exists on the javascript object
There was a problem hiding this comment.
I think they refer to the same thing: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/:checked
niklasmohrin
left a comment
There was a problem hiding this comment.
not changed since last time
janno42
left a comment
There was a problem hiding this comment.
Hovering over the disabled "make active" button on a staff semester page unexpectedly opens the modal 😅
|
Can you briefly explain what caused the modal showing up on hover? |
Bootstrap added an onmouseover-listener to the confirmation modal in the semester view. This is supposed to show the tooltip. However, when implementing show on the confirmation-modal, we somehow overrode some method, the bootstrap listener called, which means, that every onmouseover event calls our show method, which shows the modal. Renaming it to showModal fixes the issue. |
|
I see, how annoying |
|
I suppose bootstrap's idea of these attributes is that they take full ownership over this element, so a |
niklasmohrin
left a comment
There was a problem hiding this comment.
I didn't thoroughly check the implementation now, except that I saw some console log and an empty line being added. I will come back to that later. In the meantime, please perform a quick self-review to sort out these small things.
For the tests, I have some questions.
(I also unresolved a previous discussion, please take a look)
| cancel_button[0].click() | ||
| time.sleep(1) |
There was a problem hiding this comment.
Are the sleep calls really needed? I thought that selenium waits until all events triggered by the click are over?
Either way, in this case here we can also press Esc instead of clicking on the button. Then the modal will close instantly, and this is standard browser behavior, so selenium should definitely know what is happening
| modal = self.selenium.find_elements(By.CSS_SELECTOR, "confirmation-modal.mark-no-answer-modal") | ||
| self.assertEqual(len(modal), 2) | ||
| cancel_button = modal[0].shadow_root.find_elements(By.CSS_SELECTOR, "section.button-area > button") |
There was a problem hiding this comment.
why the [0]? I think what we really want to do is something along the lines of confirmation-modal.mark-no-answer-modal:has(dialog:open)
| # Waiting for element_to_be_clickable does not work unfortunately | ||
| time.sleep(1) |
There was a problem hiding this comment.
but does something not work then? the only thing we need is that the event handlers from the start are set, right? so why do we need to wait again here?
| textarea.send_keys("b") | ||
| button.click() | ||
| assert_modal_visible_and_close() |
There was a problem hiding this comment.
I don't understand, we replace 'a' with 'b' and click again and then the modal shows again; what is the test? Isn't the interesting bit that it does not show if we edit textareas somewhere else?
fixes #2302