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

New: Add option to disable popup and show error inline (fixes #12) #13

Merged
merged 15 commits into from
Oct 28, 2024

Conversation

swashbuck
Copy link
Contributor

@swashbuck swashbuck commented Oct 15, 2024

Fixes #12

New

  • Adds _showAsPopup option to enable the instruction error popup. When false (and by default), the error is shown inline instead.

Dependencies

adaptlearning/adapt-contrib-core#593 ✅ Merged
adaptlearning/adapt-contrib-core#595 ✅ Merged

Testing

  1. In course.json, disable error popups as follows:
"_instructionError": {
    "_isEnabled": true,
    "_showAsPopup": false,
    "title": "Instructions",
    "body": "{{#if instruction}}{{{instruction}}}{{else}}Please complete the question and then select Submit.{{/if}}"
}
  1. Click on a question's Submit button before it is ready for submission (ex. an MCQ item choice has not been made)

Expected result

The instruction text is highlighted.

image

@swashbuck swashbuck self-assigned this Oct 15, 2024
@swashbuck swashbuck added the enhancement New feature or request label Oct 15, 2024
@swashbuck
Copy link
Contributor Author

swashbuck commented Oct 15, 2024

@kirsty-hames Will you have a look at this and let me know what you think? I'm especially interested on your thoughts on accessibility of the solution (ex. should we use an aria-live attribute, etc). Thanks!

README.md Outdated Show resolved Hide resolved
Co-authored-by: Oliver Foster <[email protected]>
@swashbuck

This comment was marked as off-topic.

properties.schema Outdated Show resolved Hide resolved
@oliverfoster

This comment was marked as off-topic.

@swashbuck
Copy link
Contributor Author

Probably out of scope for this issue...

Definitely out of scope. Would be relatively easy to do with that component level class.

Created #14 for this.

Copy link

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well thanks @swashbuck. I've added some notes to an earlier comment querying the use of aria-live. There's good reasoning for your implementation so unless we hear otherwise I don't think aria-live is necessary.

@swashbuck
Copy link
Contributor Author

This works well thanks @swashbuck. I've added some notes to an earlier comment querying the use of aria-live. There's good reasoning for your implementation so unless we hear otherwise I don't think aria-live is necessary.

Thanks, @kirsty-hames !

@swashbuck
Copy link
Contributor Author

swashbuck commented Oct 21, 2024

It wonder if we should move the 'has-error` code into core so that the components can implement their own error styling?

@oliverfoster @kirsty-hames I'm going to create a Core PR for this per the discussion in #14 .

Update: created adaptlearning/adapt-contrib-core#594

Copy link

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retested with core PRs #593 and #595 and works as expected thanks 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

@oliverfoster
Copy link
Member

Version bump now required @swashbuck

@swashbuck swashbuck merged commit 0233e6b into master Oct 28, 2024
@swashbuck swashbuck deleted the issue/12 branch October 28, 2024 14:51
Copy link

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to disable popup and show error inline
4 participants