-
Notifications
You must be signed in to change notification settings - Fork 16
New: Add priority labels to templates and schemas (fix #816) #817
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
base: master
Are you sure you want to change the base?
Conversation
|
Ready for review. |
|
Should the icons be defined in the json? I'm not sure what the standard is there. CSS only or json define class name? |
I like the idea of setting this in the JSON config. This could be two new props that would apply to all types (box menu item, article, etc). I don't feel that we would need a different icon for each type. So, we would have: Thoughts about this approach? We would need to remove it from the Vanilla theme PR as the icon is currently hard-coded in the Less. |
|
@kirsty-hames Any thoughts on the icon config proposed solution? |
| const type = this.constructor.type; | ||
|
|
||
| // Early exit if not a supported content type | ||
| if (!['menuItem', 'page', 'article', 'block', 'component'].includes(type)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const type = this.constructor.type; | |
| // Early exit if not a supported content type | |
| if (!['menuItem', 'page', 'article', 'block', 'component'].includes(type)) { | |
| const SUPPORTED_TYPES = ['menuItem', 'page', 'article', 'block', 'component']; | |
| const type = this.constructor.type; | |
| // Early exit if not a supported content type | |
| if (!SUPPORTED_TYPES.includes(type)) { |
Consider extracting supported types for maintainability.
I'm happy with the above suggestion thanks @swashbuck. When I've seen optional/progress labels used in projects, I've only seen icons used as suggested (no icon for optional, exclamation mark for required). Whilst I can't see much scope for changing the icons at least it's future proofing for customisation. |
|
Hey @swashbuck, just thinking on this from an a11y perspective, whilst the optional/required labels are accessible, they don't have context of the content they refer to. As per my suggestion in the PLP optional content visual indicator PR, is it worth adding the optional / required labels to the model headings like we already do for complete/incomplete? Alternatively, we could add the model title to the optional/required labels as hidden labels. However, I think it makes sense to update the model headings for the following reasons:
If we did do this then we should set the priority labels to |
Fix #816
📋 To-do
New
Notes
Dependencies
❓ Questions
In this example, there is no displayed title for the optional Graphic component so the optional label is not shown (1). Then, the optional label has an exclamation mark (2).

Configuration
In course.json, add the following to
_globalsand enable/disable labels as desired.The following example configuration will show optional labels only for menu item and page headings. In contentObjects.json, set
"_isOptional": trueto make content objects optional.