Skip to content
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

Draft
wants to merge 1 commit into
base: dev/29-sodalite
Choose a base branch
from

Conversation

maufcost
Copy link
Contributor

WIP

I've got a couple of proposals for this issue besides just showing the warnings.

  1. Which warning type would you prefer? I'd go with the second, but the first one looks simpler and less button-like.
    Screen Shot 2022-01-24 at 08 47 18
    Screen Shot 2022-01-24 at 08 54 31

  2. 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?

  3. 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

@maufcost maufcost changed the base branch from master to dev/27-pyrite January 24, 2022 14:25
@FrenjaminBanklin
Copy link
Contributor

My opinions:

  1. I would probably prefer the first example rather than the second - error messages elsewhere in the editor present with dark red text on top of whichever component is relevant, we should probably be consistent with that where possible.
  2. In the interest of simplicity I think a green check mark indicating validity is unnecessary, since that much is assumed by the absence of the error message. On the other hand, I could see this being expanded to every action type which takes a node ID for the general purpose of indicating that a valid node ID has been given, since there doesn't seem to be any validation at the moment.
  3. I can see an argument for standardizing input heights, but I can also see one for keeping them as-is since the font size for text inputs is larger than that of drop-down selections. It would be a separate issue if we decided to do anything at all.

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?

@maufcost
Copy link
Contributor Author

  1. Perfect. That's what I thought too.
  2. and 3. Totally agree :)

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 :)

@FrenjaminBanklin
Copy link
Contributor

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.

@maufcost
Copy link
Contributor Author

Closing in favor of #1935 and #1960

@maufcost maufcost closed this Jan 26, 2022
@maufcost maufcost reopened this Feb 3, 2022
@maufcost
Copy link
Contributor Author

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.

@FrenjaminBanklin
Copy link
Contributor

I could see this being extended to other triggers with ID inputs as well. For example, nav:goto would make sure that the provided ID matches a page and focus:component would check that the provided ID matches up to a node on the same page as the trigger.

@FrenjaminBanklin FrenjaminBanklin changed the base branch from dev/27-pyrite to dev/29-sodalite August 26, 2022 20:16
@stale
Copy link

stale bot commented Jun 18, 2023

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.

@stale stale bot added the stale label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn authors about invalid assessment ids
2 participants