Improve login layout with proper labels#1266
Improve login layout with proper labels#1266nooreldeensalah wants to merge 3 commits intocanonical:mainfrom
Conversation
|
Thanks @nooreldeensalah
For consistency with the SSH interface, shouldn't a label also be prepended to the URL here, like so: |
dfce212 to
e1d3bb7
Compare
|
I've updated it as suggested @edibotopic, I initially followed the suggestion from this comment regarding the label becoming long on the issue. Let me know if any more changes are needed! This is how it looks like now (from |
Thanks. I think it's more visually consistent. The URL is first mentioned many lines before the QR, so it's no harm to be maximally explicit about what we are referring to, so that the connection is clear and the user can scan the text efficiently. But also, I'm not sure there is any practical advantage to centering the text for the URL and Code below the QR; if anything, it makes the text less readable. And the original comment was about the string being too long when centering is applied. So, I think this would be fine: I think that it makes more sense to have I'm not testing this, but I would also suggest (as shown in my example above) adding a newline between the QR and the strings below (if it doesn't exist already) for clearer visual separation. Any issue with the reasoning above @adombeck ? |
|
Current layout after the latest commit |
Not at all. It makes sense to me, even though I was initially a bit hesitant to replace the center alignment with left alignment, because I found it visually appealing. However, I agree that, when combined with the labels, the left alignment makes it more readable. |
pam/internal/adapter/nativemodel.go
Outdated
There was a problem hiding this comment.
The linter complains about an ineffectual assignment to firstQrCodeLine in this line. It's correct, the variable is not used anymore, you now directly use m.uiLayout.GetContent() as the URL - which makes sense to me, because we pass the content as the URL and also use it to generate the QR code. I wonder why we tried to use the first line of the rendered QR code instead. Do you remember, @3v1n0?
| qrcodeView = append(qrcodeView, fmt.Sprintf("Code: %s", code)) | ||
| } |
There was a problem hiding this comment.
Let's add another space between the URL: label and the URL, to make it align with the code, as @edibotopic suggested in #1266 (comment):
| qrcodeView = append(qrcodeView, fmt.Sprintf("Code: %s", code)) | |
| } | |
| qrcodeView = append(qrcodeView, fmt.Sprintf("URL: %s", m.uiLayout.GetContent())) | |
| qrcodeView = append(qrcodeView, fmt.Sprintf("Code: %s", code)) | |
| } else { | |
| qrcodeView = append(qrcodeView, fmt.Sprintf("URL: %s", m.uiLayout.GetContent())) | |
| } |
That seems reasonable since this change should only affect the command-line interface. When testing changes which involve gnome-shell / GDM, you will probably want to set up a VM and share the authd repository between your host and the VM. I can help you with that. |
Yes, please! That will hopefully fix the tests which are currently failing (beside the flaky ones - if you see the "Retry Go Tests with Coverage Collection" job fail, that's a quite reliable indication that there's actually something wrong). |
3b57eed to
18104bd
Compare
18104bd to
e802ab7
Compare
Thanks, I appreciate that! it would definitely be helpful :) I've re-generated the golden tests, and pushed them in my latest commit. And I've addressed your latest suggestions as well. Current layout after adding the whitespace: SSHQR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1266 +/- ##
==========================================
+ Coverage 85.00% 86.02% +1.01%
==========================================
Files 119 99 -20
Lines 7669 6674 -995
Branches 111 111
==========================================
- Hits 6519 5741 -778
+ Misses 1094 877 -217
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #1263
I've tested these changes on LXD container, for the QR code testing, I've used
lxc consolecommand. I'm unsure if this is the ideal way or not.Note: I've ran the tests locally with the
TESTS_UPDATE_GOLDENflag, and it edited multiple test files, which I commited to a separate branch nooreldeensalah@35b1378. Should I cherry-pick this commit to this PR?SSH (Before)
SSH (After)
QR (Before)
QR (After)