Conversation
|
@nathandyer seeking your input on cropping all the screenshots to the application window instead of showing the entire desktop, so we get more detail on what's relevant. I'd like to crop the screenshot_delete_sources_dialog.png further, so that the selected conversation and delete trashcan icon is very obvious in the screenshot. What do you think? |
|
@ChumOfChance Definitely a fan of the idea of cropping and focusing solely on the Inbox itself rather than the whole desktop. The only time I hesitate is when the Inbox spawns another window, like opening content in a DispVM. But even then, as long as the viewer is in front of the Inbox window, that's probably fine too. Another thought experiment is: is there a benefit to having the yellow window chrome surrounding the Inbox in the screenshots? Because if not, we could avoid cropping entirely (whether now or just in future screenshot updates) by running the Inbox in a dev environment and taking screenshots of the window specifically. We don't have to decide that for this round of updates, though. And +1 to cropping the delete_sources screenshot further to make the trash can more obvious. |
|
I think the yellow border s helpful. It helps orients the user in the Qubes environment, and when other windows are present, the different coloured borders are visible. And yes agreed for dispVM, basically I mean to crop to the "relevant content" which in most of these is just the application window itself. I am going to run through them and crop before review, I just wanted to check before cropping someone else's screenshots! |
…rs, fixed typo in reply screenshot
|
Some things I would like to add:
|
nathandyer
left a comment
There was a problem hiding this comment.
This is excellent, @ChumOfChance! The documentation on the new features is especially :chefs-kiss:
One small issue, and a few tiny nits/questions, but this is otherwise really to ship.
| Deleting | ||
| --------- | ||
|
|
||
| Deleting conversations |
There was a problem hiding this comment.
This is me asking a pedantic question, and the answer may very well be 'no': To me, "Deleting conversations" implies multiple conversations, but then we have a section below called "Deleting multiple conversations."
Should this be "Deleting a conversation"?
There was a problem hiding this comment.
Originally this header was also "Deleting conversations" and I didn't like having the same header in a row. But I think there's a good fix for this.
Co-authored-by: Nathan Dyer <nathandyer@fastmail.com>
|
@nathandyer I went over things with a close eye and tried to fix many consistency issues to address your comments. I also caught a few other things, so this probably warrants a full review (sorry). |
nathandyer
left a comment
There was a problem hiding this comment.
Just did another full review, and all the changes look great. I'm afraid I have no nits to pick or suggestions to add :)
Going to mark and approval, but leave this up for a bit in case anyone else wants to take a pass before it gets merged.
|
|
||
| To display a conversation in the conversation view, simply click a source in the | ||
| source list. | ||
| To display the conversation with a source in the conversation view, simply click a source in the source list. |
There was a problem hiding this comment.
This is great, I'm a big fan of this change
| SecureDrop administrator, as well as the two-factor code using the method you | ||
| have set up. If you have used SecureDrop before, these | ||
| are the same credentials that you would use to log in to the Journalist | ||
| are the same credentials that you would use to sign in to the Journalist |
There was a problem hiding this comment.
Another nice consistency catch!
|
I reviewed the docs as whole instead of the diff itself, so some of this may not be directly related to the changes in this PR; I will leave it up to the two of you as to what can be deferred or not.
|
|
Thanks @legoktm, I'll go ahead and grab the new/missing screenshots. @ChumOfChance I feel like most of these items (except perhaps the mention on the main page?) are relatively quick to add or change. Think we can fit them all in before the release? |
|
Yeah, I can fit all this in in Monday (or defer a few, I didn't want to get into changing file names since that makes the review more difficult). If @nathandyer you want to handle grabbing all the screenshots that would definitely help! |
|
No problem at all, I've added a commit with those. If you'd like to see any changes, @ChumOfChance, just say the word. |
This updates the Workstation documentation to account for the release of the new SecureDrop Inbox.
Test plan
Checklist
This change accounts for: