-
Notifications
You must be signed in to change notification settings - Fork 71
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
PR for addressing test related notes #20
Conversation
Moved data files to dedicated folder Split huge do_attempt test into individual tests running on plain and html'ed zones and items
@e-kolpakov Yup, thank you for this, it's going in the right direction. 👍 Do you still have some work you would like to do on this, or can I merge? I saw the "Work in progress" mention in the description so I haven't merged it yet. Also posting a couple of comments inline - note that you don't have to address them, it's good as is, but it's more FYI for the next iteration. Edit: Just noticed that you mentioned having only addressed the first comment on the initial PR - I'm stopping there for these additional comments then, to give you a chance to finish before discussing the second part : ) |
FEEDBACK = { | ||
0: {"correct": "Yes <b>A</b>", "incorrect": "No <b>A</b>"}, | ||
1: {"correct": "Yes <i>B</i>", "incorrect": "No <i>B</i>"}, | ||
2: {"correct": "", "incorrect": ""} |
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.
It doesn't hurt to have it, but it wouldn't be necessary to redo all the tests a second time with the HTML content imho
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.
The fix introduced one really subtle problem: zone ids are the same as their titles, so all the HTML put up in zone title gets into zone id as well. I decided that it wouldn't hurt having same set of tests that check "attempts" both for plain and html zone titles. Also, it was simper to implement. :)
@antoviaque This is a work in progress in a sense that other notes would be addressed in this PR as well. However, it's a self-sufficient changeset, so it might make sense merging it and having separate PR for other notes. |
@e-kolpakov Sounds good - merging! |
PR for addressing test related notes
…button [MCKIN-6339] DnDv2: Dotted border appear around close icon in the feedback pop-up
Related: #19
Work in progress