Add variant to file references.
#227
Conversation
|
No concern with what's here so far, but I think we need an addition to the contest package format as well - otherwise there is no persistence of this data. We already added variant there, but needs to cover multiple variant strings, and currently same image variant is expected to be x. |
There is persistence. You are always allowed to have a file reference object in the JSON files, and that would store the variant info.
Yes, and since that's not exactly the same thing, we probably should not call it the same?
What do you think about saying that the |
| | mime | string | Mime type of resource. | ||
| | width | integer ? | Width of the image. Required for files with mime type image/\*. | ||
| | height | integer ? | Height of the image. Required for files with mime type image/\*. | ||
| | variant | array of string | Intended usage hints (e.g. `light`, `dark` for images). |
There was a problem hiding this comment.
Maybe I'm the only one, but I don't think variant is a good name here as to me it suggests that the options are mutually exclusive. Also, given that this is a list of options, I'd go for a plural terms, so for lack of better, I'd say tags.
Yes, but we often have files without json or incomplete json, so would be preferable to have both options, just like we do with x.
Agreed, and I'm ok with modifying either term.
+1
Generally, yes. We only say 'should' for the x so probably just the same thing here. Variant is allowed to be anything today (and I think we want to keep that?) so I think server just looks for its known variants after splitting up the variant string by '_'. |
| - `<property>` is the property of the object, e.g. `logo`. | ||
| - `(.<variant>)` is an optional variant. Must be used when there are | ||
| multiple files to avoid collisions. | ||
| - `(.<variant>)*` is one or several optional variant tags. |
There was a problem hiding this comment.
nit: 'one or several' -> 'one or more'
There was a problem hiding this comment.
I had suggested "zero or more" (to correspond to the RE), and drop the optional part?
There was a problem hiding this comment.
Yeah, that works for me too.
| - `light`: an image intended for use on white or light backgrounds. | ||
| - `dark`: an image intended for use on black or dark backgrounds. | ||
|
|
||
| An image may list both values if it is suitable for multiple contexts. |
There was a problem hiding this comment.
nit: 'multiple contexts' -> 'both backgrounds' simpler?
| For images, a variant of `.<W>x<H>`, is interpreted as the file reference object | ||
| having the `width` property set to `<W>` and the `height` property set to | ||
| `<H>`. Every other variant is interpreted as the file reference object having | ||
| the value its `variant` property array. |
There was a problem hiding this comment.
Missing 'in': 'the value in its'
|
Discussed on the meeting today. I will fix the outstanding comments and then merge. Comment if you disagree... |
5 participants
Fixes #202