Wrap login options in accordion #8
Conversation
There was a problem hiding this comment.
Yeah, I like this a lot better. Could use some more spacing between the two options. Maybe even put a paragraph with a big fat --- or --- in between.
Comments are mostly about the HTML coding style. Please try to keep the diffs small and not needlessly reformat / wrap HTML code. I know it's hard to read as it is, but the diff with your wrapping changes just makes it much harder to see what's changing.
|
Note that I haven't actually tested any WebAuthn logins yet. But code-wise it looks like a nice a clean solution with the local storage approach. |
I tried this: Personally I don't like it, I much prefer the look with the plain panels: I also worry that the "OR" might get a bit janky when translated. For example in English, "or" and "Or" look visually unbalanced, so it should be "OR", so I put Also, this particular HTML solution depends on flexbox, which I'm not sure we assume is supported by every user's browser? This would be very finnicky to make without flexbox, so I'd much prefer to just scrap it.
I've reused the same panel component as in the main GUI - should we really introduce this spacing difference between those uses? |
|
Yes, I think a difference in spacing is justified. In the main GUI, it's a list (repetition) of the same type of thing. Here, it's a contrast between two alternatives. The horizontal line looks fine to me with its spacing. We could just leave out the "OR" text and only have the line. I agree that it's a headache in translations. Sorry, I don't know what flexbox means and what the exact usage is. But adding some space via a CSS override should be easy and portable. Just as adding a |
This padding decrease currently applies to _all_ collapsible panels, but this padding decrease is not appropriate for all collapsible panels.
This padding decrease currently applies to _all_ collapsible panels, but this padding decrease is not appropriate for all collapsible panels.
…abled" This reverts commit 0a2e6bc.
#9659) Transplanted from emlun#8 (meta-PR into #9175) by request of @acolomb (see: emlun#8 (comment)). This padding decrease currently applies to _all_ collapsible panels, but this padding decrease may not be appropriate for all collapsible panels. In particular, it will not be appropriate for the collapsible panels introduced in emlun#8.
|
Actually, one more try at the alignment stuff... This is what it would look like if we add a panel-footer section and move the button and the checkbox into that: Feel free to ignore or holler if you want me to propose the code changes if you like the look. |
|
The text is still not quite aligned correctly: But sure, you're very welcome to submit that as a meta-PR into syncthing#9175 if you prefer the look. |
This is a meta-PR into:
@acolomb I believe this should address your review: syncthing#9175 (review)