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

Fix truncation and alignment issues in conversation view #1029

Merged
merged 2 commits into from Mar 31, 2020

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Mar 31, 2020

Description

Fixes #998 (comment)
Fixes #694
Also download icon is no longer misaligned
Fixes #953

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

@sssoleileraaa
Copy link
Contributor Author

(This does not fix #815)

@sssoleileraaa sssoleileraaa force-pushed the fix-conversation-layout-widgets branch from a46e66d to 27c3ebc Compare March 31, 2020 00:34
@sssoleileraaa sssoleileraaa marked this pull request as ready for review March 31, 2020 00:34
@sssoleileraaa sssoleileraaa force-pushed the fix-conversation-layout-widgets branch from 27c3ebc to 6acf47c Compare March 31, 2020 00:36
@eloquence
Copy link
Member

Good initial results in non-Qubes environment. Before:

Screenshot from 2020-03-30 17-27-19

After:

Screenshot from 2020-03-30 17-26-57

The attachment now has a bit less padding than before the change which does in fact seem to bring it closer to the spec, which also doesn't have a ton of padding here.

@eloquence
Copy link
Member

OK the lack of padding when a file is the last thing is a bit extreme now:
Screenshot from 2020-03-30 17-41-41

@sssoleileraaa
Copy link
Contributor Author

@eloquence is that a file submission without a message?

@eloquence
Copy link
Member

Yes. Here's how that case is rendered in master:
Screenshot from 2020-03-30 17-44-03

@sssoleileraaa
Copy link
Contributor Author

good catch there needs to more padding... i'll add that back

@eloquence
Copy link
Member

More subtly, there's a small visual regression in the alignment between speech bubbles and file widgets, after a file has been downloaded -- note the alignment of "Export" before and after (the "E" should start where the speech bubble starts). Given the severity of the underlying issue, may be fine to accept to land this PR, but flagging in case it may have further unintended effects.

Before:
Screenshot from 2020-03-30 17-48-51
After:
Screenshot from 2020-03-30 17-48-25

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Mar 31, 2020

Download link no longer misaligned, see...

Before:
Screenshot from 2020-03-30 17-58-19

After:
Screenshot from 2020-03-30 17-58-50

@sssoleileraaa
Copy link
Contributor Author

Addresses #1029 (comment):

Screenshot from 2020-03-30 18-03-28

@eloquence
Copy link
Member

Can reproduce re: download link, looks like we're trading one alignment issue for another.

@sssoleileraaa sssoleileraaa force-pushed the fix-conversation-layout-widgets branch from 6acf47c to 83c4f21 Compare March 31, 2020 01:06
@sssoleileraaa
Copy link
Contributor Author

what do you mean?

@eloquence
Copy link
Member

Sorry, I mean yes, the Download link is now aligned with the speech bubble, and the Export link is not, whereas before it was the other way around (both should be aligned with the bubble).

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Mar 31, 2020

So just to be clear, this fixes the major file widget alignment issues on master, e.g.#998 (comment). The file widget would sometimes be much more misaligned than that.

That is a much bigger issue than #1029 (comment), which if you give me a few I can address.

@eloquence
Copy link
Member

eloquence commented Mar 31, 2020

Yep agreed, this is a minor issue (and not even a full regression since it fixes a previous misalignment in the "Download" case) that can be fixed in follow-up if needed.

@eloquence
Copy link
Member

Padding in "last interaction was a file upload" case now looks great:

Screenshot from 2020-03-30 18-19-01

@sssoleileraaa sssoleileraaa force-pushed the fix-conversation-layout-widgets branch from 29fb1ef to 4c5cf45 Compare March 31, 2020 01:25
@sssoleileraaa sssoleileraaa force-pushed the fix-conversation-layout-widgets branch from 4c5cf45 to 9f650b1 Compare March 31, 2020 01:27
@sssoleileraaa
Copy link
Contributor Author

Addressed #998 (comment):

@eloquence
Copy link
Member

Confirmed. Both Download and Export now perfectly aligned. :)

@sssoleileraaa
Copy link
Contributor Author

did you add extra carriage returns at the top of the second message of #1029 (comment)? should we open an issue to think about truncating leading and following newlines?

@eloquence
Copy link
Member

eloquence commented Mar 31, 2020

Ah yes, I did notice that before - that was a result of including whitespace in the clipboard, and yeah, I do think we should trim leading and trailing whitespace; I'll file a small enhancement issue for that. (Done: #1030)

@sssoleileraaa sssoleileraaa added this to Ready for Review in SecureDrop Team Board Mar 31, 2020
@sssoleileraaa sssoleileraaa changed the title WIP - fix truncation and alignment issues Fix truncation and alignment issues in conversation view Mar 31, 2020
@eloquence
Copy link
Member

Looks good to me in Qubes testing as well, all my existing sources on my production instance still render correctly, and I am no longer able to reproduce the text truncation/squishing issue.

@conorsch
Copy link
Contributor

Also confirming resolved over here. Tested on two different machines, so widget styling is slightly different.

Before

sdc-1029-repro-1

After

sdc-1029-repro-2

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Looks good with different download option states, here is a screenshot.

good_window

@kushaldas kushaldas merged commit 66a61db into master Mar 31, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Mar 31, 2020
@kushaldas kushaldas deleted the fix-conversation-layout-widgets branch March 31, 2020 10:37
@eloquence eloquence moved this from Done to I am a temporary column, ignore me in SecureDrop Team Board Mar 31, 2020
@eloquence eloquence moved this from I am a temporary column, ignore me to Done in SecureDrop Team Board Mar 31, 2020
@eloquence eloquence mentioned this pull request Apr 2, 2020
3 tasks
@eloquence eloquence mentioned this pull request Feb 8, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
4 participants