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: support for responsive menu item image sources (fixes #198) #200

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

kirsty-hames
Copy link
Contributor

@kirsty-hames kirsty-hames commented Jul 2, 2024

Fixes #198

Update

  • Use image template for consistency with other plugin images and multi device support.

New

  • Support for device breakpoint sizes: small, medium, large and xlarge.
  • When only small and large images are defined, images switch from small to large at medium (for separate mobile and desktop images).
    Note, this development is only available to the Framework at present until a core contentobject.model.schema file is updated to offer up the flexibility to the AAT. This is also the case for all other plugins that use the image template.

Testing

Please test the following use cases. All code snippets reference pre-bundled course images.

  1. Default support for src
    "_graphic": {
      "src": "course/en/images/menu-item.png",
      "alt": ""
    }
  1. Mobile and desktop images
    "_graphic": {
      "large": "course/en/images/menu-item.png",
      "small": "course/en/images/gmcq.png",
      "alt": ""
    }
  1. All device sizes
    "_graphic": {
      "xlarge": "course/en/images/vanilla-swatch.jpg",
      "large": "course/en/images/menu-item.png",
      "medium": "course/en/images/vanilla-swatch.jpg",
      "small": "course/en/images/gmcq.png",
      "alt": ""
    },

Dependencies

Now that .boxmenu-item__image-container is a <span> (not <div>), Vanilla margin-bottom is no longer supported (inline elements only support horizontal margins). Instead, I'd suggest applying the margin to .boxmenu-item__details as per below. See Vanilla issue adaptlearning/adapt-contrib-vanilla#520

.boxmenu-item {
  &__image-container + &__details {
    margin-top: @menu-item-image-margin;
  }
}

- set alt text as 'null' to prevent aria-label being applied to img. Maintain alt text reading in the menu item details instead.
this only needs setting if different to classNamePrefixes
- attribution not currently supported in existing Boxmenu
- the reading order would render attribution before menu item title and image alt text which isn't logical
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@guywillis
Copy link
Contributor

Not sure if it's worth mentioning but this development is only available to the Framework at present until a core contentobject.model.schema file is updated to offer up the flexibility to the AAT ( if that is something we want to do ).

@kirsty-hames
Copy link
Contributor Author

Not sure if it's worth mentioning but this development is only available to the Framework at present until a core contentobject.model.schema file is updated to offer up the flexibility to the AAT ( if that is something we want to do ).

Thanks @guywillis. That's right and it's the same case for all other plugins that also use the image template. I'll update the PR message to note this.

Plugin README, example.json and schemas only include the image sources they were initially developed to support although they all support the full device range within the FW. For example, GMCQ supports small and large, whilst Accordion only supports src.
If we want to extend support for AAT would we be best to do this globally for all plugin images or at least look at this holistically?

@guywillis
Copy link
Contributor

If we want to extend support for AAT would we be best to do this globally for all plugin images or at least look at this holistically?

We'd want to implement a consistent approach globally but is outside the scope of this PR. One to bring to the weekly stand up.

@kirsty-hames
Copy link
Contributor Author

If we want to extend support for AAT would we be best to do this globally for all plugin images or at least look at this holistically?

We'd want to implement a consistent approach globally but is outside the scope of this PR. One to bring to the weekly stand up.

Just FYI I've raised an issue for this adaptlearning/adapt-contrib-core#563

Copy link
Contributor

@guywillis guywillis left a comment

Choose a reason for hiding this comment

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

👍

@oliverfoster oliverfoster merged commit 65ca0c1 into master Jul 15, 2024
1 check passed
@oliverfoster oliverfoster deleted the issue/198 branch July 15, 2024 11:16
github-actions bot pushed a commit that referenced this pull request Jul 15, 2024
# [7.1.0](v7.0.2...v7.1.0) (2024-07-15)

### New

* Support for responsive menu item image sources (fixes #198) (#200) ([65ca0c1](65ca0c1)), closes [#198](#198) [#200](#200)
Copy link

🎉 This PR is included in version 7.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
dependencies Pull requests that update a dependency file enhancement released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu item support for responsive image sources
4 participants