Extra breakpoints required. Additionally, should images cascade as we're making this change.

[StuartNicholls]

Following the breakpoint update in core, we need to add in an extra large node for headers and backgrounds.

See issue #431

Additional to this issue, as the change included a 'mobile first' approach, ideally, this would work so it follows the same functionality as css and the mobile first approach. So setting a 'small' image would cascade to all the sizes medium/large/xlarge. Setting 'medium' would cascade to large/xlarge. However, this is slightly different to how it currently works. Setting a 'small' only applies to an image to the 'small' breakpoint.

[oliverfoster]

@guywillis 's solution to the inheritance issue was to use the keyword inherit explicitly, as doing so preserves the current behaviour and extends it with inheritance

[StuartNicholls]

Ideally, this would behave in exactly the same way as the CSS, so it automatically 'inherits', then to stop it cascading we'd set 'none' - so that would stop it cascading.

But, yes there is something to be said for preserving the behaviour. And @guywillis suggestion seems good to me if thats what we want to do.

[oliverfoster]

The other backward compatible solution is to have a checkbox next to each _isMediumInherited, _isLargeInherited and _isXLargeInherited, to make the AAT UI a little clearer. The AAT doesn't currently have a nice way of representing keywords as image urls:

1. One has to "Select an External Asset" to get a text input to enter a keyword.

2. Enter a keyword instead of a url

3. Then it's not clear that a keyword has been used because there is an image icon.

@guywillis were there any other keywords that would be useful or are booleans ok for inherit behaviour? The boolean style, in order to accomodate the AAT UI, is less simple, but at least it doesn't confuse keywords with urls which would otherwise necessitate an AAT UI update.

You'd suggested:

{
  "_xlarge": "inherit" // inherits from _large
  "_large": "https://...", 
  "_medium": "", // doesn't inherit from _small, so the null string is significant
  "_small": "https://..."
}

Mine is a little more confusing from a json perspective, as it would be possible to set a value and inherit at the same time, but it makes the AAT more friendly:

{
  "_xlarge": "https://...", // this value is not significant as inheritance applies
  "_isXLargeInherited": true,  // inherits from _large
  "_large": "https://...", 
  "_isLargeInherited": false,  // does not inherit from _medium
  "_medium": "",
  "_isMediumInherited": false,  // does not inherit from _small
  "_small": "https://..."
}

[StuartNicholls]

Does AAT allow you to add options? So it starts with just small, then you add a breakpoint? Not sure if thats more confusing tbh

[StuartNicholls]

To me this makes more sense

{
  "_small": "https://..."
  "_medium": "", // inherits from _small
  "_large": "https://...", // overwrites small (or medium)
  "_xlarge": "none" // doesn't inherit from _large
}

Not sure about the boolens seems confusing to me

[StuartNicholls]

The other option is we could just add the additional breakpoint and leave is as it currently functions. There is some logic to that. It doesn't necessarily have to behave in the same way as the css.

[oliverfoster]

The other option is we could just add the additional breakpoint and leave is as it currently functions. There is some logic to that. It doesn't necessarily have to behave in the same way as the css.

Two separate issues? It'd be easy to add the xlarge config and behaviour without adding inheritance or impacting the introduction of inheritance later.

[StuartNicholls]

@oliverfoster, yep thats what I was asking at the top really. I'll do exactly that.

[StuartNicholls]

@oliverfoster I've updated the description to focus on the inheritance issue. New issue and PR for adding it a breakpoint in the description.

[StuartNicholls]

So I've updated PR #432 with the extra breakpoint. What are peoples thoughts on this? I'm tempted to say leave as is, but if there is an elegant way of doing it in AAT then perhaps consider it further? @taylortom ?

[oliverfoster]

It either requires a new bit of UI in the AAT, some AAT UI amends or some inelegant schema, as far as I can see atm.

The principle of inheritance is sound. I'm very much not sure about inheritance by default, firstly as it breaks the existing behaviour and secondly without a new bit of UI or a UI amend to explain the inheritance concept to new user, the behaviour would only be implied. It would be an inexplicable exception to the way all of the other properties work in the AAT at present.

As I'd previously proposed, boolean toggles negate the debate about whether to use an inclusive or exclusive keyword to define the new behaviour, as the boolean is always available via a checkbox and must always be either true or false. Boolean toggles would not change the default behaviour or require any AAT UI amends or additions.

I am absolutely up for making a preliminary design for a new AAT input type, that is both breakpoint aware and follows models of inheritance / override. It would seem to be reasonably possible and extremely useful to have a single UI to be able to:

• choose assets vs breakpoints, with inheritance
• predefined classes vs breakpoints, with inheritance

[StuartNicholls]

Absolutely agree. Perviously, I was having problems visualising what it would look like in AAT. But, I think if Im following your suggestion correctly, that makes sense. The issue I was focusing on was understanding it in the framework, which is actually less important.

100% agree, with your last points also. There is a wider issue with how we're going to handle applying different images to the breakpoints but also layout and heading sizes etc etc. So, perhaps fixing this here will be a good idea. I need to have another look at AAT, I might pop some mock-ups of my thoughts in pictorial form. @oliverfoster Would you be able to do a really rough mock-up of the checkbox idea, pretty sure I get it now, just to be sure.

[oliverfoster]

Sketch of image inheritance without breaking or UI amends: