-
Notifications
You must be signed in to change notification settings - Fork 34
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
Shows invalid assessment id warning #1953
base: dev/29-sodalite
Are you sure you want to change the base?
Shows invalid assessment id warning #1953
Conversation
My opinions:
All that being said, I find myself wondering why there's an ID input for this action at all. It seems like any module will only ever have the one assessment - so if we know that the action will start an attempt for an assessment, and there's only ever going to be one of them, it seems unnecessary to require authors to provide an ID since there's only one possible valid value. Maybe I'm missing something? |
Correct. As I was working on this issue, I thought about the same thing. I believe I created this issue with Zac back then when we thought there could be more than one assessment/obo module in the future. However, there aren't any prospects for that just yet, and I don't think there will be for a long time since we have other goals right now. Since there isn't too much work on this issue, I can close it and create another one to delete those assessment id inputs since we only have one assessment/module now :) |
Especially since #1935 will theoretically remove the need for authors to copy and paste node IDs themselves, that sounds fine with me. The work you've done so far could still be helpful in a future where we need to validate author input on actions, but I can't really predict what/when those might be. |
I am just waiting for the latest issue @jpeterson976 is addressing regarding the new dropdown on trigger modals to be merged in order to add this issue's validation logic there. |
I could see this being extended to other triggers with ID inputs as well. For example, |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep our backlog under control, but we thank you for your contributions. |
WIP
I've got a couple of proposals for this issue besides just showing the warnings.
Which warning type would you prefer? I'd go with the second, but the first one looks simpler and less button-like.
I was also thinking about showing a green checkmark on the right of the text input or make the input border green when the user types a correct assessment id. Thoughts?
Something that bothers me on the trigger modal is the difference in height between the text inputs and the select tags. I was thinking about making them having the same height. Thoughts?
Fixes #1625