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

Improve styling of error panel #8739

Closed
bampakoa opened this issue Apr 9, 2020 · 13 comments · Fixed by #8960
Closed

Improve styling of error panel #8739

bampakoa opened this issue Apr 9, 2020 · 13 comments · Fixed by #8960

Comments

@bampakoa
Copy link
Contributor

bampakoa commented Apr 9, 2020

Hey all! One of the areas that I think takes some improvement is the error panel. Whenever I see it in a forum question or Github issue, it feels a bit scary with its dark background 😄. In conjunction with the light color fonts, they create a high contrast situation that is not preserved very well.

I think that the error panel should take an overall re-styling that will be closer to Cesium colors (green, blue) and improve the user experience for users. If you think that this is a good idea, I would love to help with this and submit a suggestion PR.

@hpinkos
Copy link
Contributor

hpinkos commented Apr 9, 2020

Great suggestion @bampakoa! I don't think the themeing of that panel has ever been revisited

If you want to take a stab at it, I would be happy to review a PR! You can find the CSS here: https:/CesiumGS/cesium/blob/master/Source/Widgets/CesiumWidget/CesiumWidget.css#L27

@bampakoa
Copy link
Contributor Author

bampakoa commented Apr 9, 2020

@hpinkos Thanks! Definitely I will do! How about exporting the showErrorPanel in a separate widget https:/CesiumGS/cesium/blob/master/Source/Widgets/CesiumWidget/CesiumWidget.js#L606? Do you think this is feasible?

@hpinkos
Copy link
Contributor

hpinkos commented Apr 9, 2020

I'm not sure if that's entirely necessary since it's pretty much tied into the CesiumWidget. I think it's a better idea to start with a PR with just the styling changes so we don't have to worry about waiting for feedback on the other changes holding up the PR from being merged

@bampakoa
Copy link
Contributor Author

@hpinkos here is a preview of the styling https://bit.ly/2VBca55.

I would also prefer the following:

  1. Add a close (x) button at the top right corner
  2. Convert the OK button to Copy to copy the error to the clipboard

@OmarShehata
Copy link
Contributor

I like the idea of a copy - since that's often the common immediate next action, to report this error to someone (whether it's the application developer or opening an issue here).

I'm not sure about a close button or having "Ok" close the window - since that implies the application can carry on, but CesiumJS will only throw these errors when the application is an invalid state (as far as I'm aware). Sandcastle examples will use a window.alert for communicating if something is unsupported but when the application is allowed to carry on: https://sandcastle.cesium.com/index.html?src=3D%20Tiles%20Point%20Cloud%20Shading.html.

@bampakoa
Copy link
Contributor Author

I'm not sure about a close button or having "Ok" close the window - since that implies the application can carry on

@OmarShehata

I agree with your point 😃 . This is a UX pattern that is generally followed in dialogs and pop-ups. It depends on the context of the error panel. If this is meant to be a dialog then maybe we should think to include both, a Cancel and a Close button.

Another aspect is that the user should be able to continue after an error occurs. Not necessarily in a fully functional state but maybe to reproduce a behavior 😉

@bampakoa
Copy link
Contributor Author

@hpinkos it's been a while since this issue has been discussed. Do you have any additional thoughts on this?

@OmarShehata
Copy link
Contributor

One thing I wanted to respond to here:

Another aspect is that the user should be able to continue after an error occurs. Not necessarily in a fully functional state but maybe to reproduce a behavior wink

I actually forgot that the current CesiumJS error panel is dissmissable, I almost never dismiss it because I think it's almost always been a fatal error in my experience. This makes me think we should have separate types of errors in the library, like warning dialogs that are dismissable and fatal errors that are not.

CesiumJS generally uses console logs for "one time warnings". What we do in Sandcastle for "dismissable warnings" is just window.alert(). I think improving this may be a separate effort but just wanted to note this down here for context.

@hpinkos
Copy link
Contributor

hpinkos commented May 26, 2020

@bampakoa I like your suggestions in #8739 (comment)

I would probably make the panel red though, so it reads more like an error. You can use #d69d93 for the background color and #510c00 for the text color

@hpinkos
Copy link
Contributor

hpinkos commented May 26, 2020

@OmarShehata If there's a runtime error that triggers this panel to appear, there should already be something logged to the console so we don't need to add anything else to handle that.

@bampakoa
Copy link
Contributor Author

bampakoa commented Jun 1, 2020

@hpinkos your suggestions look great! Thanks for the feedback 😃 I have made some adjustments that you can take a look here.

Notice that I have also added padding in the title of the error message so that it can stand up from the stack trace and be more easily distinguishable.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 12, 2020

Looking good @bampakoa !

I would just make one change to make the whole panel the same color:
image

Could you open a pull request with your changes?

@bampakoa
Copy link
Contributor Author

bampakoa commented Jun 13, 2020

Sure @hpinkos 😀

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

Successfully merging a pull request may close this issue.

3 participants