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

Update: Move notify title above body text (fix #586) #587

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Conversation

swashbuck
Copy link
Contributor

@swashbuck swashbuck commented Oct 1, 2024

Fix #586

Update

  • Moves the notify title directly above the body text
  • Applies to Notify popups that use notifyPopup.hbs
  • Adds a .notify__text wrapper to the body and text area

Questions

  1. Should this be a configuration option or just the default behavior?
  2. Are there any use cases that this will break?
  3. What are the disadvantages for using this layout? Is it a bad idea?

Testing

  1. Add feedback with images. For example:
"_feedback": {
  "title": "Feedback",
  "_correct": {
    "title": "Correct feedback text.",
    "body": "That’s correct. Aliquip id nisi sunt do sint. Laboris nisi nisi Lorem tempor non proident. Nulla laboris ad incididunt irure officia cillum incididunt esse sunt laborum.",
    "_imageAlignment": "left",
    "_graphic": {
      "_src": "course/en/images/correct.jpg",
      "alt": "Correct"
    }
  },
  "_incorrectFinal": {
    "title": "Incorrect feedback text.",
    "body": "That’s not right. Elit dolore Lorem proident mollit ipsum pariatur dolore. Labore amet enim minim excepteur duis. Nulla eu mollit culpa veniam culpa sint adipisicing. Anim cupidatat do do mollit pariatur et.",
    "_imageAlignment": "right",
    "_graphic": {
      "_src": "course/en/images/incorrect.jpg",
      "alt": "Incorrect"
    }
  }
},
  1. Test without images as well.

@swashbuck swashbuck self-assigned this Oct 1, 2024
@swashbuck swashbuck added enhancement New feature or request question Further information is requested labels Oct 1, 2024
@swashbuck swashbuck marked this pull request as ready for review October 1, 2024 21:46
@oliverfoster
Copy link
Member

Update

a. Moves the notify title directly above the body text when a graphic is present
b. Apples to Notify popups that use notifyPopup.hbs
c. Adds a .notify__text wrapper to the body and text area

Questions

  1. Should this be a configuration option or just the default behavior?
  2. Are there any use cases that this will break?
  3. What are the disadvantages for using this layout? Is it a bad idea?

a. Do it all of the time not just when a graphic is present?

  1. No config for this, it's a really abstract thing to describe.

I like it.

@swashbuck
Copy link
Contributor Author

a. Do it all of the time not just when a graphic is present?

This change is made.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

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.

This works great thanks @swashbuck.

I think it makes sense to have the text and image container as sibling elements. This improves the default layout and further styling can be applied via flex properties to change the alignment e.g. center the text to the image like the screenshot in #586

@swashbuck swashbuck changed the title Update: Move notify title when graphic is present (fix #586) Update: Move notify title above body text (fix #586) Oct 15, 2024
@oliverfoster oliverfoster merged commit 8557584 into master Oct 15, 2024
@oliverfoster oliverfoster deleted the issue/586 branch October 15, 2024 15:24
github-actions bot pushed a commit that referenced this pull request Oct 15, 2024
# [6.58.0](v6.57.2...v6.58.0) (2024-10-15)

### Update

* Move notify title above body text (fix #586) (#587) ([8557584](8557584)), closes [#586](#586) [#587](#587)
@oliverfoster
Copy link
Member

🎉 This PR is included in version 6.58.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 question Further information is requested released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notify: Move title directly above body text
4 participants