-
Notifications
You must be signed in to change notification settings - Fork 214
feat(ui): implement activity item error dialog #2584
Conversation
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.
This needs a couple of tweaks IMO
-
it's not obvious it's a link or where you can/should click
-
The cursor needs to change to the pointer on hover which would as help identify it as a link
-
If we are going to use a dialog, the title of the dialog should probably not be
An error has occurred
- that makes me think an unrelated error occurred caused by me clicking on the link
There was a problem sending your payment
would be a better title
But actually I'm not convinced that a dialog is the best way to display this.
The dialog pattern feels more appropriate to display details of an error that just happened, rather than for retroactively displaying details about errors that happened in the past.
5af9224
to
ffef774
Compare
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.
Code looks good and works well. I have 2 change requests though:
-
The cursor should change to a pointer on hover to make it more obvious that you can click on the text.
-
I don't think the modal should auto close when you copy the text. I asked to copy the error message, not close the window and by having the window closed automatically I now have to find the item in the activity list and click on it again to get back to where I was.
ffef774
to
9f07a74
Compare
@mrfelton updated |
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.
Tested ACK 9f07a74
Description:
Resolves #2580
How Has This Been Tested?
Manually
Screenshots (if appropriate):
Types of changes:
UX enhancement
Checklist: