-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
add qr code, add screen if JavaScript is disabled #25
Conversation
…d was wondering why only one branch hides sth)
…affecting lots of other things
@adbenitez the tests are failing, probably because of some changed class names, idk, i have not really an idea what to do about that. can you fix them? otherwise, i suggest to drop them, we do not have such UI tests even for more important things afaik (desktop ...) and they stand quite in the way of rapid development esp. of devs not into these frameworks EDIT: i added a commit to drop the failing test, see commit message for details |
we now have a fallback for non-js, this makes all other elements hidden by default, so the existing tests will fail. if someone deep into the used testing framework want to recreate the tests, step forward :) tbh, i am not sure if there is super-much value for tests while the whole UI is in development and subject to change, it slows down things - and in case of this small repo, it is also easy to test things manually - many problems, as the %23 mismatch etc. are not covered by tests anyways.
a1e7146
to
f6d14e4
Compare
<form> | ||
<noscript> | ||
<div> | ||
<h2><b>Say hello 👋 to ...</b> you've disabled JavaScript, no problem:</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe discard the "say hello to ..." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is that the user can get an idea about what the following steps are about directly in the headline; therefore i did not start with "JavaScript disabled"
<section> | ||
<p> | ||
<div id="say-hello" class="hidden"> | ||
<h2> | ||
<span id="join">Say hello to</span> | ||
<b id="name">NAME</b> | ||
👋 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this emoji should go at the start instead of the end, otherwise looks like part of the group or user name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if ppl think, it belongs to the name, this would not be really an issue if it is not at the end.
i think it looks nice and friendly this way; it was the outcome of some discussions with rosano in the initial PR, i would not change this here - if it becomes an issue, we can think over
index.html
Outdated
</section> | ||
|
||
<details> | ||
<summary>I have Delta Chat installed on another device</summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use instead:
<summary>I have Delta Chat installed on another device</summary> | |
<summary>Click here if you have Delta Chat installed on another device</summary> |
at least to me it looks a bit not obvious the text is clickable otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added a commit, however, avoided to make the text longer.
moreover, i make the link look like a link on hover - i also tried some blueish, but that removes attention from the buttons, so, let's see.
it is still not clear if ppl are more consuming the link on the same or on another device, we'll know better if we have some feedback, maybe we can put the focus more on the QR code in general then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow!!! nice improvements!! ❤️
…, however, that removes attention from the buttons
this PR adds an option to open the chat from another device by scanning a QR code and adds a fallback screen if JavaScript is disabled.
the "local flow" is left as is for now, the PR just adds a link at the bottom.
we could put the QR code more into focus, eg. sth. as "tap or scan", however, this may also result in ppl just scanning and not "seeing" the "installation step 1". also, it is a bit unclear, if ppl really open the invite-link very often on another device, so, that needs a bit feedback anyways. we can change that easily always as needed.
moreover, this PR cleans up a little, avoiding complicated CSS relations.
The QR Code:
Before / After if JavaScript is disabled:
localisation can be done in another PR once the wording is settled (there are also other things strings missing)
closes #21
closes #26