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

Change styles of inline error messages for dialog #3532

Closed
wants to merge 2 commits into
base: master
from

Conversation

5 participants
@http-request
Contributor

http-request commented Dec 7, 2017

Change styles for inline error messages for dialog. Issue is #2052

@iAmWillShepherd

This comment has been minimized.

Show comment
Hide comment
@iAmWillShepherd

iAmWillShepherd Dec 7, 2017

Contributor

@http-request could you post some before and after images for your change?

Contributor

iAmWillShepherd commented Dec 7, 2017

@http-request could you post some before and after images for your change?

@http-request

This comment has been minimized.

Show comment
Hide comment
@http-request

http-request Dec 7, 2017

Contributor

Before:

screen shot 2017-12-07 at 15 27 03

screen shot 2017-12-07 at 15 26 51

screen shot 2017-12-07 at 15 26 41

Contributor

http-request commented Dec 7, 2017

Before:

screen shot 2017-12-07 at 15 27 03

screen shot 2017-12-07 at 15 26 51

screen shot 2017-12-07 at 15 26 41

@http-request

This comment has been minimized.

Show comment
Hide comment
@http-request

http-request Dec 7, 2017

Contributor

After:

screen shot 2017-12-07 at 21 39 38

screen shot 2017-12-07 at 21 39 17

screen shot 2017-12-07 at 21 39 05

Contributor

http-request commented Dec 7, 2017

After:

screen shot 2017-12-07 at 21 39 38

screen shot 2017-12-07 at 21 39 17

screen shot 2017-12-07 at 21 39 05

@j-f1

This comment has been minimized.

Show comment
Hide comment
@j-f1

j-f1 Dec 7, 2017

Member
the third screenshot

Is there a way to remove the padding on this warning? The dialog already has padding, so it would look nicer IMHO if the padding was 🔥ed.

Member

j-f1 commented Dec 7, 2017

the third screenshot

Is there a way to remove the padding on this warning? The dialog already has padding, so it would look nicer IMHO if the padding was 🔥ed.

@http-request

This comment has been minimized.

Show comment
Hide comment
@http-request

http-request Dec 8, 2017

Contributor

Updated:

screen shot 2017-12-08 at 17 20 41

Contributor

http-request commented Dec 8, 2017

Updated:

screen shot 2017-12-08 at 17 20 41

@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Dec 8, 2017

Member

@desktop/design should review this before it lands.

Member

joshaber commented Dec 8, 2017

@desktop/design should review this before it lands.

@niik

This comment has been minimized.

Show comment
Hide comment
@niik

niik Dec 8, 2017

Contributor

Hey @http-request, thank you so much for wanting to contribute to GitHub Desktop!

As I said in #2052 (comment) I don't think the solution to #2052 is to change the style of the DialogError component. The problem pointed out in that issue has to do with improper use of it.

See https://github.com/desktop/desktop/blob/master/docs/technical/dialogs.md#errors

The DialogError component, if used, must be the first child element of the Dialog itself.

DialogError is meant to be used when immediate validation (i.e. on text changed) is not realistic (due to performance), when the validation result can not be guaranteed (i.e. race conditions, file system is a good example) or when it's not suitable from a UX perspective. In other words. These errors should normally only appear after you've submitted the form in the dialog.

In this specific instance of the directory creation failing what we need to do is move the DialogError component up to the top of the dialog (and probably tweak the message). We should not change the icon, these are definitely errors, not warnings.

I'm going to close this PR but If you'd like to start a PR to make the changes I outlined above we'd be happy to take a look!

Contributor

niik commented Dec 8, 2017

Hey @http-request, thank you so much for wanting to contribute to GitHub Desktop!

As I said in #2052 (comment) I don't think the solution to #2052 is to change the style of the DialogError component. The problem pointed out in that issue has to do with improper use of it.

See https://github.com/desktop/desktop/blob/master/docs/technical/dialogs.md#errors

The DialogError component, if used, must be the first child element of the Dialog itself.

DialogError is meant to be used when immediate validation (i.e. on text changed) is not realistic (due to performance), when the validation result can not be guaranteed (i.e. race conditions, file system is a good example) or when it's not suitable from a UX perspective. In other words. These errors should normally only appear after you've submitted the form in the dialog.

In this specific instance of the directory creation failing what we need to do is move the DialogError component up to the top of the dialog (and probably tweak the message). We should not change the icon, these are definitely errors, not warnings.

I'm going to close this PR but If you'd like to start a PR to make the changes I outlined above we'd be happy to take a look!

@niik niik closed this Dec 8, 2017

@http-request

This comment has been minimized.

Show comment
Hide comment
@http-request

http-request Dec 8, 2017

Contributor

😢 why is the world so cruel 😢

Contributor

http-request commented Dec 8, 2017

😢 why is the world so cruel 😢

@http-request http-request deleted the http-request:refresh-style-for-inline-error-messages branch Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment