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

refactor dialog #3764

Merged
merged 4 commits into from
Mar 24, 2023
Merged

refactor dialog #3764

merged 4 commits into from
Mar 24, 2023

Conversation

fufesou
Copy link
Collaborator

@fufesou fufesou commented Mar 24, 2023

Mainly move dialogs from mobile path to common path.

Signed-off-by: fufesou <shuanglongchen@yeah.net>
Signed-off-by: fufesou <shuanglongchen@yeah.net>
Signed-off-by: fufesou <shuanglongchen@yeah.net>
Signed-off-by: fufesou <shuanglongchen@yeah.net>
@rustdesk rustdesk merged commit c3dfcd6 into rustdesk:master Mar 24, 2023
@rustdesk
Copy link
Owner

rustdesk commented Mar 24, 2023

@grummbeer do you have any idea about this dialog design which is for login to headless linux? Different from normal login, remote linux user/password is required for headless linux. I still think current dialog does explain clearly, the users can be confused by the additonal user name and password.

@rustdesk
Copy link
Owner

Screenshot_20230324_173831

@grummbeer
Copy link
Contributor

Let me try it first. How it feels and how it behave on (wrong) input.

I still think current dialog does explain clearly,

sorry, which?

the users can be confused by the additonal user name and password.

On the first glace, the message from toolip could placed directly obove the system login to avoid question marks. And below that, a heading for rustdesk password, even the placeholder already says so.

@grummbeer
Copy link
Contributor

usernameController == null || usernameController == null

both sides are the same

@rustdesk
Copy link
Owner

usernameController == null || usernameController == null

both sides are the same

@fufesou

@rustdesk
Copy link
Owner

Let me try it first. How it feels and how it behave on (wrong) input.

I still think current dialog does explain clearly,

Sorry, i meant the new dialog above.

@rustdesk
Copy link
Owner

rustdesk commented Mar 24, 2023

On the first glace, the message from toolip could placed directly obove the system login to avoid question marks.

Yes, @fufesou place it under title directly with smaller smaller light font, and change title to original one (before this PR). You even do not put a big question mark there, no guys know moving mouse there to show a tooltip.

@rustdesk
Copy link
Owner

rustdesk commented Mar 24, 2023

and this seperator is not eye-catching enough, when I say put a seperator there, you should have some thinking also, not just for finishing an assigned job.

image

@grummbeer
Copy link
Contributor

According to material https://m3.material.io/components/dialogs/guidelines#fb75c64c-81c1-45ea-b97b-eb0bcb4c5dc9

The headline should smaller and a kind of heading/description goes to both sections. Section 1. Login to system, section 2. Login to rustdesk.

@rustdesk
Copy link
Owner

Section 1. Login to system, section 2. Login to rustdesk.

Yes, @fufesou

@rustdesk
Copy link
Owner

rustdesk commented Mar 24, 2023

Seperator line can be remove if so, I think, with heading (and its padding).

@grummbeer
Copy link
Contributor

Seperator line can be remove if so, I think.

yes separation made by heading.

@grummbeer
Copy link
Contributor

sorry if this is a stupid question but …

final username = usernameController?.text.trim() ?? '';
final password = passwordController?.text.trim() ?? '';
final peerPassword = peerPasswordController?.text.trim() ?? '';
if (peerPasswordController != null && peerPassword.isEmpty) return;
gFFI.login(
// username,
// password,
id,
peerPassword,
remember,
);

final username and final password not used due commented out in login() arguments.

  1. What happen with them on submit?

  2. Can you clarify the situation with a comment? or similar
    It is irritating when stumple upon such situations because it is not clear what is intended. Just a forgotten test? Something unfinished? Wanted as hint for later work? Can it be saftly removed? Or going the save way and leaveing it untouched to prevent the author from being pissed?

@fufesou
Copy link
Collaborator Author

fufesou commented Mar 24, 2023

final username and final password not used due commented out in login() arguments.

They should be sent to the controlled side. These variables are keet here for the near future work.

@fufesou
Copy link
Collaborator Author

fufesou commented Mar 24, 2023

Thanks for your reminder, I'll add some comments.

@grummbeer
Copy link
Contributor

Thank you for the clarification!

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

3 participants