Fix: Allow overrides on content objects (fixes #12)

[swashbuck]

Fixes #12

Fix

• Adds _isHidden option to hide the logo on the main menu (in course.json)
• Content objects will inherit logo's _src and alt values from course.json
• Removes version number from README.md

[kirsty-hames]

I'm finding that the contentObject config is preventing the course config being applied globally. In a Framework build, we typically only add config when we want to override a model so this isn't an issue. In the AAT, all config exists and the default contentObject config _navigationLogo._isEnabled: false is removing the logo from all pages unless I enable and link up an image src etc for each contentObject.

Looking at where we have similar inheritance in Adapt we could try either of the following:

• An inheritance option like we have in Trickle.
• A hide option like we have in homebutton.

[swashbuck]

I'm finding that the contentObject config is preventing the course config being applied globally.

Thanks, Kirsty! This will need a bit of a refactor then. The AAT always complicates things 😛

@kirsty-hames @StuartNicholls Do we need the ability to override the logo image for specific pages? Or are we fine with a single logo image that's always defined at the course level? Then, it can be shown or hidden on specific pages and the menu.

[swashbuck]

@kirsty-hames @StuartNicholls I've added a new _isHidden property (only to the documentation, example.json, and schemas) in bfb1768. Let me know if this will work, and I'll implement it in the code.

[swashbuck]

@kirsty-hames @StuartNicholls I went ahead with the _isHidden option for course.json. PR description is updated above. This should work with the AAT now (tested in legacy AAT).

[kirsty-hames]

a single logo image that's always defined at the course level? Then, it can be shown or hidden on specific pages and the menu.

Apologies for the delayed response but I agree with your suggestion, a single logo image with the option show/hide for specific pages and menu.

[kirsty-hames]

• Changes src to _src to align with _mobileSrc

Looking at image source defined elsewhere in Adapt, src does seem to be the majority. e.g. _graphic.src for Boxmenu items and components. Should we be using src and mobileSrc for consistency?

Note, boxmenu logo graphic does use _src but this was the only non src instance I could find.

[StuartNicholls]

• Changes src to _src to align with _mobileSrc

Looking at image source defined elsewhere in Adapt, src does seem to be the majority. e.g. _graphic.src for Boxmenu items and components. Should we be using src and mobileSrc for consistency?

Note, boxmenu logo graphic does use _src but this was the only non src instance I could find.

Should this not be _small and _large so it reflects the breakpoints?

[StuartNicholls]

We could do with improving/rethinking the CSS in my humble opinion (or maybe I'm missing something) but perhaps thats a separate issue anyways.

[oliverfoster]

The _ is short for not translatable. All resource URLs should start with _. src without the underscore is a mistake that was replicated.

[kirsty-hames]

The _ is short for not translatable. All resource URLs should start with _. src without the underscore is a mistake that was replicated.

Just so I'm clear @oliverfoster, are we saying to use _src moving forward and it's a known issue that we have src used elsewhere?

[StuartNicholls]

@kirsty-hames I think that's just an issue here, but yep, the underscore indicates 'do not translate' etc. My point is that elsewhere we use the breakpoint terminology, fixing this will be a breaking change no? Might as well update?

https://github.com/adaptlearning/adapt-contrib-graphic/blob/master/example.json

[kirsty-hames]

Should this not be _small and _large so it reflects the breakpoints?

I'd agree, it does seem we use src for a single image source and large/small for responsive image sources in Adapt (noting these should be prefixed by _). If this is something that needs discussion perhaps it's best to leave the breaking change out of this PR and raise as a separate issue? And keep this PR for the content object overrides only.

[kirsty-hames]

@kirsty-hames @StuartNicholls I went ahead with the _isHidden option for course.json. PR description is updated above. This should work with the AAT now (tested in legacy AAT).

This is working as expected thanks @swashbuck.

[oliverfoster]

The _ is short for not translatable. All resource URLs should start with _. src without the underscore is a mistake that was replicated.

Just so I'm clear @oliverfoster, are we saying to use _src moving forward and it's a known issue that we have src used elsewhere?

Yeas. Always use an _ unless the property is text and translatable.

[oliverfoster]

@kirsty-hames I think that's just an issue here, but yep, the underscore indicates 'do not translate' etc. My point is that elsewhere we use the breakpoint terminology, fixing this will be a breaking change no? Might as well update?

https://github.com/adaptlearning/adapt-contrib-graphic/blob/master/example.json

Yes, which is why it hasn't yet been fixed in the graphic schema / example, please don't change the properties in adapt-contrib-graphic schema or example. It is fixed in the image.jsx template.
https://github.com/adaptlearning/adapt-contrib-core/blob/b20e6f85edd597007e2846152d64996e07f1ee10/templates/image.jsx#L12-L21

[StuartNicholls]

@oliverfoster Completely missed there wasn't an underscore there!

[swashbuck]

Ok, I've reverted the src property change and have raised a new PR (#16). Otherwise, let me know if you need any other changes.

[swashbuck]

Ok to merge, @StuartNicholls ?

[StuartNicholls]

@kirsty-hames correct me if I'm wrong but I think the logic is wrong here. @swashbuck I think we want the logo to appear on content object pages if there is no config present for framework builds. So example.json should also be:

// contentObjects.json
"_navigationLogo": {
    "_isEnabled": false
}

I think this reflects the functionality of homebutton ?

Also, the default nav order is 1, which is possibly correct, but the default spacer order in framework is 0, so the logo appears on the right hand side. Wasn't it on the left perviously? Maybe spacer should be 50 in framework?

[swashbuck]

I think we want the logo to appear on content object pages if there is no config present for framework builds.

@StuartNicholls Ok, I've made this change in d6cbc15. Content objects no longer require a _navigationLogo property and will be enabled by default if the config is in course.json.

[swashbuck]

Also, the default nav order is 1, which is possibly correct, but the default spacer order in framework is 0, so the logo appears on the right hand side.

@StuartNicholls Will you file a new ticket for this one? It may be part of a larger discussion about nav order default values. Thanks!

[kirsty-hames]

Also, the default nav order is 1, which is possibly correct, but the default spacer order in framework is 0, so the logo appears on the right hand side.

@StuartNicholls Will you file a new ticket for this one? It may be part of a larger discussion about nav order default values. Thanks!

Setting the nav order to 0 does mean the logo displays to the left of the spacer but agree this would be better raised as a core issue to review the nav order as a collection. On projects I typically use increments of 10 (minimum) as it gives a bit of flexibility when appending new nav items. I think this approach would be useful for setting defaults as it allows wriggle room for non-core plugins to fit around the core elements.

[kirsty-hames]

I think we want the logo to appear on content object pages if there is no config present for framework builds.

@StuartNicholls Ok, I've made this change in d6cbc15. Content objects no longer require a _navigationLogo property and will be enabled by default if the config is in course.json.

Can confirm this is working as described. Testing on Mac Safari, FF, Chrome and iPhone

[kirsty-hames]

Perhaps add the default display to the last line?

e.g. The logo can be shown on the menu, on specific pages, or everywhere as default.

[swashbuck]

Done in e62e714

[kirsty-hames]

Working as expected thanks @swashbuck. Re-tested on Mac Safari, FF, Chrome and iPhone.

[github-actions]

🎉 This PR is included in version 3.0.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[guywillis]

Just a note to say the _isHidden property should probably be called something a bit more explicit to imply what the config option does. At present, _isEnabled and _isHidden both imply similar functionality whilst something like _isHiddenOnMenu is more descriptive.

[swashbuck]

Just a note to say the _isHidden property should probably be called something a bit more explicit to imply what the config option does. At present, _isEnabled and _isHidden both imply similar functionality whilst something like _isHiddenOnMenu is more descriptive.

Thanks, @guywillis . Will you mind raising a new ticket for this? Alternatively, we have a ticket ( #16 ) to align image source properties, so we could tack it on to that?

[kirsty-hames]

Just a note to say the _isHidden property should probably be called something a bit more explicit to imply what the config option does. At present, _isEnabled and _isHidden both imply similar functionality whilst something like _isHiddenOnMenu is more descriptive.

Thanks, @guywillis . Will you mind raising a new ticket for this? Alternatively, we have a ticket ( #16 ) to align image source properties, so we could tack it on to that?

Both the readme text and the schema title note this hides the logo on the menu.

[guywillis]

Both the readme text and the schema title note this hides the logo on the menu.

It's great to have the functionality defined in documentation but I do think the naming of config options should try to succinctly describe functionality too.

Thanks, @guywillis . Will you mind raising a new ticket for this? Alternatively, we have a ticket ( #16 ) to align image source properties, so we could tack it on to that?

Sure, I'll raise a new ticket. Ticket here