Skip to content
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

GPII-3411: Adding schemas for UIO+ solutions entries. #679

Merged
merged 4 commits into from Oct 4, 2018

Conversation

Projects
None yet
4 participants
@jobara
Copy link
Contributor

jobara commented Oct 3, 2018

@jobara

This comment has been minimized.

Copy link
Contributor Author

jobara commented Oct 3, 2018

@the-t-in-rtf would you like to review this PR to add the schemas for UIO+

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 3, 2018

Could one of the admins verify that these changes are reasonable to test? If so, please reply with "ok to test".

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

the-t-in-rtf commented Oct 3, 2018

@jobara, I'm happy to review and comment but someone else will need to actually merge.

}
}
},
"capabilitiesTransformations": {

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Oct 3, 2018

Member

Thank you for alphabetising these, it makes it much easier to keep track of things IMO.

This comment has been minimized.

Copy link
@jobara

jobara Oct 3, 2018

Author Contributor

Yah, i should have done that from the beginning. It was getting pretty hard to find things.

"gw",
"bbr"
]
}

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Oct 3, 2018

Member

This won't be a valid schema until you add "enumLabels", one for each option.

Long term raw strings will be moved to a message bundle, but I wouldn't expect you to do that in this pull. Just enter the raw strings for each label and I'll move it later on.

"yellow",
"green",
"pink"
]
}

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Oct 3, 2018

Member

Need enumLabels throughout.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

the-t-in-rtf commented Oct 3, 2018

With the exception of the missing "enumLabels" for all enum properties, this looks good to me. Also, ok to test.

"schema": {
"title": "Contrast Theme",
"description": "Change text and background colors.",
"type": "string",

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Oct 3, 2018

Member

This type information is redundant for an enum. The only small danger is that the validation output would include two failures if you (for example) passed a number, as it's neither a string nor one of the accepted values. Although it's mostly harmless, I'd argue for leaving it out as it makes for simpler feedback. Copying @amb26 to get his opinion on this small bit of redundancy.

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

I'm happy for this to be omitted

"schema": {
"title": "Selection Theme",
"description": "Change the highlight colour for text selections.",
"type": "string",

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Oct 3, 2018

Member

Another redundant type for an enum, whatever we decide, these should all match.

@jobara

This comment has been minimized.

Copy link
Contributor Author

jobara commented Oct 3, 2018

@the-t-in-rtf, I've added the enumLables. Waiting to here back from @amb26 about the "string" types.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 3, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 3, 2018

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

the-t-in-rtf commented Oct 3, 2018

The new enumLabels look good, @jobara.

@jobara

This comment has been minimized.

Copy link
Contributor Author

jobara commented Oct 3, 2018

@amb26 and @the-t-in-rtf this is ready for more review and/or merging

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

the-t-in-rtf commented Oct 3, 2018

Looks good to me.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 3, 2018

@amb26 amb26 merged commit 7fbdfb3 into GPII:master Oct 4, 2018

1 check passed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.