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 |
Contest_API.md
Outdated
| | 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 '_'. |
deboer-tim
left a comment
deboer-tim
left a comment
There was a problem hiding this comment.
Added a few minor suggestions, but approving.
|
Discussed on the meeting today. I will fix the outstanding comments and then merge. Comment if you disagree... |
niemela
dismissed stale reviews from deboer-tim and nickygerritsen
via
4d9f6fe
Co-authored-by: Jaap Eldering <eldering@users.noreply.github.com>
| | 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). No meaning must be implied or inferred from the order of the elements. |
There was a problem hiding this comment.
I slightly prefer tag, but ok either way.
| organizations/kth.se/logo.160x160.png | ||
| organizations/baylor.edu/logo.56x56.png | ||
| organizations/baylor.edu/logo.160x160.png | ||
| organizations/tudelft.nl/logo.dark.56x56.png |
There was a problem hiding this comment.
Should maybe add a one example with multiple variants/tags?
There was a problem hiding this comment.
Yes, we should. I suggest doing it later.
|
We discussed this at the meeting. I will merge after the meeting it gets approved. |
|
I will change name to |
Fixes #202