Fix some screenshot JSON issues #2454
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Marcono1234
commented
Feb 20, 2022
| @@ -169,7 +169,7 @@ | |||
| }, | |||
| { | |||
| "title": "Check certificates for validity before a trip", | |||
| "anchor": "ios_check_validity_certificate", | |||
| "anchor": "android_check_validity_certificate", | |||
There was a problem hiding this comment.
Arguably this is not much better because the screenshots below are actually for iOS; however this is the Android section.
Maybe this section should be removed completely for Android then, in case no screenshots exist for it?
Marcono1234
commented
Feb 20, 2022
| @@ -169,7 +169,7 @@ | |||
| }, | |||
| { | |||
| "title": "Zertifikate vor einer Reise auf Gültigkeit prüfen", | |||
| "anchor": "ios_check_validity_certificate", | |||
| "anchor": "android_check_validity_certificate", | |||
There was a problem hiding this comment.
See comment above
|
I support all you say and your work to 100%, especially the workflow you created! However, it was agreed to not change old screenshot versions, see #1660 (comment). You could certainly discuss with the maintainers whether this agreement could be thought of if there is an automatic checking process, etc. |
larswmh
approved these changes
Mar 9, 2022
luisoos
approved these changes
Apr 9, 2022
|
Hey @svengabr, please decide how to proceed with this PR. Thanks. |
dsarkar
approved these changes
Jun 1, 2022
|
@Marcono1234 Thanks for the PR, sorry for the delay! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes some screenshot JSON issues. This is not extensive; there are still remaining issues not fixed by this pull request:
alttexts (Generate screenshotalttexts automatically (or omit them) #2453)E.g. version 2.10 using screenshots of version 2.9 (maybe in some cases this is acceptable, since screenshots from the correct version are missing).
assets/img/faq/hr_en/assets/img/faq/hr_deThis is related to the above. These files are not part of the versioned screenshot folders, therefore either they will become stale, or when updated they will be incorrect for the archived versions referencing them, because back then the screen looked differently.
E.g. the Android screenshots page showing iOS screenshots.
I have created a proof of concept GitHub workflow in https://github.com/Marcono1234/cwa-website/tree/marcono1234/screenshots-json-validation (not properly cleaned up yet) for checking the JSON files for such errors. It also reports the problems in a pull request comment, as seen here (note that normally it would only show some of the issues if the screenshot JSON files were modified). Please let me know if you are interested in this, but keep in mind that the validation scripts for this might add a maintenance burden, and might therefore not be justified for this small improvement they add.