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

Added a nice UI for the E2E-enabled account first connect #1241

Merged

Conversation

ivan-cukic
Copy link
Contributor

Instead of immediately popping up the mnemonic dialogue,
only show a notification bar on the account setup page.

Screenshot_20190509_003510

For the cases where the user does not want to use E2E,
this is significantly less intrusive than the old approach.

This is related to #917 and #753

Instead of immediately popping up the mnemonic dialogue,
only show a notification bar on the account setup page.

For the cases where the user does not want to use E2E,
this is significantly less intrusive than the old approach.
@ivan-cukic
Copy link
Contributor Author

This is what it looks like on Windows (ignore the system exclude file message)

Screenshot_20190509_102027

@camilasan
Copy link
Member

What do you think @jancborchardt?

@jancborchardt
Copy link
Member

Good stuff @ivan-cukic! Only details:

Thanks a lot! 🚀

@ivan-cukic
Copy link
Contributor Author

I first wrote it with the dashes, and then saw that it is written without dashes in a few places, so I ended up removing them. Will put them back.

WRT 'setup', I wanted to ask whether anyone has a better idea for the button title as it is not really setting anything up.

Yes, the icon is awkward on Windows. It uses QStyle::SP_DialogCloseButton. I'll see to replace it with the one you proposed.

@misch7
Copy link
Member

misch7 commented May 10, 2019

Nice work 👍

WRT 'setup', I wanted to ask whether anyone has a better idea for the button title as it is not really setting anything up.

How about "Enable" or "Activate"?

(Perhaps "Enable..." / "Activate..." to signal there's something more to adjust than just a click.)

While writing this I feel I'd personally prefer "Enable" rather than "Activate"^^

- Text changed to "Enable..." instead of "Setup"
- The close icon follows NC style
- "end-to-end" instead of "end to end"

Signed-off-by: Ivan Čukić <ivan.cukic@kde.org>
@jancborchardt
Copy link
Member

@misch7 to be even more descriptive, the button should include what it will enable, like "Enable encryption".

@misch7
Copy link
Member

misch7 commented May 14, 2019

@misch7 to be even more descriptive, the button should include what it will enable, like "Enable encryption".

Even though this could be clear to understand from the notification context (see screenshot) I think you're right, this would improve the button to be super clear to the user. And it would still be short enough.

@ivan-cukic
Copy link
Contributor Author

Message changed.

{
ui->encryptionMessage->setText(tr("This account supports end-to-end encryption"));

QAction *mnemonic = new QAction(tr("Enable encryption..."), this);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
QAction *mnemonic = new QAction(tr("Enable encryption..."), this);
QAction *mnemonic = new QAction(tr("Enable encryption"), this);

The ellipsis is not really needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was requested by @camilasan to indicate that it opens up a new window

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I wouldn’t expect that from "…" a simple ellipsis. Also other buttons like "Add folder sync connection" and "Edit ignored files" which open a new window don’t do that. So I’d cut it for less noise.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looking good for me design-wise, nice work @ivan-cukic! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants