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

Fix: Expanded feedback to add images and more control (fixes #222) #223

Merged
merged 7 commits into from
Oct 19, 2022

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Aug 17, 2022

fixes #222

This behaviour is fully backward compatible if the old style of _feedback objects are used. The new style feedback can be used in the new AAT and if building by hand, it will not be supported in the classic AAT

New

  • Added object style for feedback states, at _feedback._correct, _feedback._partlyCorrectNotFinal, _feedback._partlyCorrectFinal, _feedback._incorrectNotFinal ,_feedback._incorrectFinal and for _items[].feedback
{
  "altTitle": "",
  "title": "",
  "body": "Incorrect feedback final",
  "_classes": "",
  "_imageAlignment": "left",
  "_graphic": {
    "_src": "course/en/images/single-width.png",
    "alt": "test",
    "attribution": ""
  }
}

Removed

  • QuestionModel functions to be replaced with simpler getFeedback function
    • setupCorrectFeedback
    • setupPartlyCorrectFeedback
    • setupIncorrectFeedback
    • setupAttemptSpecificFeedback

Fix

  • Block helper compare can now be used as a boolean helper

Testing

  1. Requires Update: Switch to model.getFeedback allowing more complex feedback objects adapt-contrib-tutor#79
  2. Use the following json for 'c-60':
{
        "_id": "c-60",
        "_parentId": "b-40",
        "_type": "component",
        "_component": "mcq",
        "_classes": "",
        "_layout": "left",
        "_attempts": 3,
        "_questionWeight": 1,
        "_canShowModelAnswer": true,
        "_shouldDisplayAttempts": false,
        "_isRandom": true,
        "_selectable": 1,
        "title": "Multiple Choice Question",
        "displayTitle": "Multiple Choice Question 1sel 1att new",
        "body": "In what year was the first recorded instance of a large scale assessment that consists solely of multiple choice questions?",
        "instruction": "Choose one option and select Submit.",
        "_items": [
          {
            "text": "1917",
            "_shouldBeSelected": true
          },
          {
            "text": "1888",
            "_shouldBeSelected": false,
            "feedback": "Item specific 1888 incorrect feedback"
          },
          {
            "text": "1953",
            "_shouldBeSelected": false,
            "feedback": {
                "altTitle": "",
                "title": "Item specific 1953 incorrect feedback",
                "body": "Item specific 1953 incorrect feedback",
                "_imageAlignment": "right",
                "_graphic": {
                  "_src": "course/en/images/single-width.png",
                  "alt": "test",
                  "attribution": ""
                }
            }
          },
          {
            "text": "1977",
            "_shouldBeSelected": false,
            "feedback": "Item specific 1977 incorrect feedback"
          }
        ],
        "_feedback": {
          "altTitle": "",
          "title": "Feedback global title",
          "_classes": "",
          "_correct": {
            "altTitle": "",
            "title": "",
            "body": "Correct feedback final",
            "_classes": "",
            "_imageAlignment": "left",
            "_graphic": {
              "_src": "course/en/images/single-width.png",
              "alt": "test",
              "attribution": ""
            }
          },
          "_incorrectNotFinal": {
            "altTitle": "",
            "title": "",
            "body": "Incorrect feedback not final",
            "_classes": "",
            "_imageAlignment": "right",
            "_graphic": {
              "_src": "course/en/images/single-width.png",
              "alt": "test",
              "attribution": ""
            }
          },
          "_incorrectFinal": {
            "altTitle": "",
            "title": "",
            "body": "Incorrect feedback final",
            "_classes": "",
            "_imageAlignment": "left",
            "_graphic": {
              "_src": "course/en/images/single-width.png",
              "alt": "test",
              "attribution": ""
            }
          }
        },
        "_pageLevelProgress": {
          "_isEnabled": true,
          "_isCompletionIndicatorEnabled": true
        }
    }

Requires
ref adaptlearning/adapt-contrib-tutor#79

Todo

  • Schemas - will do in a separate pr (needs wider coordination)
  • Vanilla course - will do in a separate pr (adapt_framework)
  • Documentation - (wiki updates needed)

@oliverfoster oliverfoster marked this pull request as ready for review September 6, 2022 09:29
@kirsty-hames
Copy link
Contributor

The testing json, 'c-60' provided above is missing a closing bracket for _incorrectNotFinal._graphic

Copy link
Contributor

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

No problems testing this but I've raised a couple of queries around the popup layout.

less/core/notify.less Outdated Show resolved Hide resolved
templates/notifyPopup.hbs Outdated Show resolved Hide resolved
- Update text/image layout to 60/40 for right and left aligned images (for desktop view)
- Stack text/image for mobile view
- Update alignment selectors for consistency with other plugin popup alignment classes (Hotgraphic, Hotgrid etc)
- Removed legacy alignment classes (line 53 onwards). I can't see that classes ever got appended to .notify__content for these to work. Both component level and feedback level _classes get applied to .notify only.
- for consistency where 'image' has been used elsewhere
@kirsty-hames
Copy link
Contributor

I've just committed my suggestions to notify.less and notifyPopup.hbs. A couple of things to note:

  • Currently, the image rendering is dependent on _imageAlignment so I think we should set a default of right? Otherwise if someone forgets to set this the image wont display.
  • Although I've updated the alignment selectors for consistency with other popup alignment classes. Should we be using the same attribute _imageAlignment elsewhere instead of classes? For AAT users, having config is more user friendly then having to apply classes. Perhaps there's an argument to make core plugin classes config and keep classes to the theme?

Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

Working as expected with FW v5.22.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@oliverfoster oliverfoster merged commit b34d4c5 into master Oct 19, 2022
@oliverfoster oliverfoster deleted the issue/222 branch October 19, 2022 09:00
github-actions bot pushed a commit that referenced this pull request Oct 19, 2022
## [6.20.5](v6.20.4...v6.20.5) (2022-10-19)

### Fix

* Expanded feedback to add images and more control (fixes #222) (#223) ([b34d4c5](b34d4c5)), closes [#222](#222) [#223](#223)
@github-actions
Copy link

🎉 This PR is included in version 6.20.5 🎉

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.

Feature request: Feedback image and generic text
5 participants