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

Added screenshots #45

Merged
merged 2 commits into from
May 28, 2021
Merged

Added screenshots #45

merged 2 commits into from
May 28, 2021

Conversation

tdulcet
Copy link
Collaborator

@tdulcet tdulcet commented May 20, 2021

Fixes #25 (screenshots only).

Four of the Thunderbird screenshots will need to be added in a follow up PR because of #25 (comment).

The issue in #21 (review) is showing in some of the screenshots of the options page. I also had to modify my local copy of the extension to workaround TinyWebEx/AutomaticSettings#11.

I was unsure of the expected format and use of the amoScreenshots.csv file, as your example in #25 (comment) is an invalid CSV file, as it is using semicolons instead of commas. The name implies that it is only for AMO, although I listed all the screenshots in it.

Please let me know if you would like me to redo any of the screenshots.

@tdulcet tdulcet requested a review from rugk May 20, 2021 12:53
@rugk
Copy link
Owner

rugk commented May 20, 2021

is an invalid CSV file, as it is using semicolons instead of commas. The name implies that it is only for AMO, although I listed all the screenshots in it.

Well… semicolons are also a common separator in CSVs, but yeah, it does not matter since it is never automaticaly parsed. YOu actually need to manually copy it into the AMO description. 🙃

Anyway, thanks a lot and I’ll have a look/review that soon (please ping me/request a review if I should forget it).

Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • as for optionsPageChrome.png is there any way to make it show the full page somehow? Using the web dev tools to create a screenshot of all options?
    Or use the dev tools to make your screen 10k pixel height or so, so the
    Or if nothing else works, maybe manually open the URL of the add-on and /options/options.html (though that is maybe bad as you loose the Chrome UI then)
    The use case of that screenshot is after all so that users can see all options in advance (before installing the extension) – it’s not good if they only see the first X ones.
  • As for the options, did you really choose the default options? IIRC live preview is not enabled by default, but in your screenshots it is. Or am I wrong?
  • Nice, good idea to include the casing options!
  • Uhm funny how in Chrome/ium some squared letters are red. Do you think this warrants a new issue here? After all, it's confusing at least… (but maybe also hard to work around…)
  • As for the first sentence I would have ended it with “them… how awesome!” or so instead of “them… great” as "the term “great” is already used in the sentence before. However, it’s okay, so you don’t need to recreate the screenshots just for that, only if you mind/anyway change them, you could include it! 😃

Apart from that looks really cool and thanks again for doing it! 🎉

assets/screenshots/amoScreenshots.csv Outdated Show resolved Hide resolved
assets/screenshots/amoScreenshots.csv Outdated Show resolved Hide resolved
@tdulcet
Copy link
Collaborator Author

tdulcet commented May 21, 2021

  • as for optionsPageChrome.png is there any way to make it show the full page somehow?

Sure, see TinyWebEx/AutomaticSettings#12 (comment). Your AutomaticSettings library would likely need to add some CSS to specify a width for the options page and possibility a height. This can also be an issue with popups in Chrome. See here for the CSS I used for my Colab Autorun and Connect add-on. I can of course add some CSS to my development copy of this add-on, although I figured we wanted the screenshots to show what it would actually look like for users.

  • As for the options, did you really choose the default options? IIRC live preview is not enabled by default, but in your screenshots it is. Or am I wrong?

Oops, looks like I forgot to reset the settings. I will retake those two screenshots.

  • Uhm funny how in Chrome/ium some squared letters are red. Do you think this warrants a new issue here? After all, it's confusing at least… (but maybe also hard to work around…)

Yeah, see rugk/awesome-emoji-picker#93 (comment). Several of the letters for a couple of the Unicode fonts (see here and here) are also emojis and Chrome decided to render them as such. I do not think there is much we can do about it.

  • As for the first sentence I would have ended it with “them… how awesome!” or so instead of “them… great” as "the term “great” is already used in the sentence before. However, it’s okay, so you don’t need to recreate the screenshots just for that, only if you mind/anyway change them, you could include it! 😃

OK. I tried to select that existing “great”, but the context menu would cover almost all the text, so I just added the new “great” at the end for consistency, so all the screenshots of the text transformation feature would still use the word “great”. I obviously could not select any of the existing words in the second sentence, as they were all already transformed. Adding “how awesome!” instead is a good idea, so I will do that if I need to redo those screenshots…

@tdulcet
Copy link
Collaborator Author

tdulcet commented May 21, 2021

Four of the Thunderbird screenshots will need to be added in a follow up PR because of #25 (comment).

I found a workaround for this, so I could at least take the screenshots.

@rugk
Copy link
Owner

rugk commented May 21, 2021

Sure, do what you think is a good idea.

Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Images LGTM. 😃

assets/screenshots/amoScreenshots.csv Outdated Show resolved Hide resolved
@tdulcet tdulcet requested a review from rugk May 22, 2021 07:30
@tdulcet
Copy link
Collaborator Author

tdulcet commented May 22, 2021

I think this PR is the last piece needed for us to release v0.5, since #4 is obviously not going to get fixed for a while. #36 and #43 included some important bug fixes we should publish/release as soon as possible.

However, one thing I noticed when taking the screenshots of the options page is that the RandomTips are still not working:
image
We probably should reopen #23. There are also a couple other issues listed in the third paragraph at the top of the PR that we probably should fix first…

@rugk
Copy link
Owner

rugk commented May 28, 2021

There are also a couple other issues listed in the third paragraph at the top of the PR that we probably should fix first…

Sure, would you mind to create issues for them? Feel free to assign me if it is related to the RandomTips lib, so I’ll fix it then.

@rugk rugk merged commit 224b71f into main May 28, 2021
@rugk rugk deleted the screenshots branch May 28, 2021 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screencat and screenshots
2 participants