Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix UX issues with bug report dialog #1863

Merged
merged 16 commits into from
Apr 30, 2018

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Apr 27, 2018

  • Make it use BaseDialog / DialogButtons (also gives it has a top-right 'x' &
    escape to cancel works)
  • Stop misusing the 'danger' CSS class on the buttons. There is nothing dangerous
    about submitting logs.
  • Continued campaign against 'Click here' links.

Fixes element-hq/element-web#6622

Based off #1860 because I keep needing to add things to DialogButtons and it would conflict.

 * Make the 'delete my data' button not the default
 * Make it red
 * Give it a confirmation dialog
 * Remove the 'cancel' button: what does it mean to cancel an error?
   In this case, it tried again and almost certainly got the same error.
 * Remove the top-right 'x' and don't cancel on esc for the same reason.
 * Move 'send bug report' to a button rather than a 'click here' link
 * Add a 'refresh' button which, even if it's no more likely to work,
   will at least look like it's doing something (it's mostly so if you
   don't have a bug report endpoint, there's still a button other
   than the one that deletes all your data).
…andling' into dbkr/fix_session_restore_fail_dialog_ux
…andling' into dbkr/fix_session_restore_fail_dialog_ux
 * Make it use BaseDialog / DialogButtons (also gives it has a top-right 'x' &
   escape to cancel works)
 * Stop misusing the 'danger' CSS class on the buttons. There is nothing dangerous
   about submitting logs.
 * Continued campaign against 'Click here' links.

Fixes element-hq/element-web#6622
…alog_ux' into dbkr/bug_report_dialog_basedialog
 * Make the 'delete my data' button not the default
 * Make it red
 * Give it a confirmation dialog
 * Remove the 'cancel' button: what does it mean to cancel an error?
   In this case, it tried again and almost certainly got the same error.
 * Remove the top-right 'x' and don't cancel on esc for the same reason.
 * Move 'send bug report' to a button rather than a 'click here' link
 * Add a 'refresh' button which, even if it's no more likely to work,
   will at least look like it's doing something (it's mostly so if you
   don't have a bug report endpoint, there's still a button other
   than the one that deletes all your data).
 * Make it use BaseDialog / DialogButtons (also gives it has a top-right 'x' &
   escape to cancel works)
 * Stop misusing the 'danger' CSS class on the buttons. There is nothing dangerous
   about submitting logs.
 * Continued campaign against 'Click here' links.

Fixes element-hq/element-web#6622
@lukebarnard1 lukebarnard1 force-pushed the dbkr/bug_report_dialog_basedialog branch from 74651b9 to a9b6db3 Compare April 30, 2018 12:41
Copy link
Contributor

@lukebarnard1 lukebarnard1 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@@ -250,6 +250,7 @@ textarea {
.mx_Dialog button.danger, .mx_Dialog input[type="submit"].danger {
background-color: $warning-color;
border: solid 1px $warning-color;
color: $accent-fg-color;
Copy link
Contributor

Choose a reason for hiding this comment

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

From a code perspective, fine. From a UI perspective though, this text should very much be white.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently the text on all the red buttons is black in the dark theme

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. Sorry, I should've looked at what the colours actually are.

@@ -41,6 +42,13 @@ export default React.createClass({
// cancelled (BaseDialog itself only calls this with false).
onFinished: PropTypes.func.isRequired,

// Whether the dialog should have a 'close' button that will
// cause the dialog to be cancelled. This should only be set
// to true if there is nothing the app can sensibly do if the
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be false?

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth mentioning the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, good spot

hasCancel={false}
>
<button onClick={this._onClearStorageClick} className="danger">
{ _t("Clear Storage and Sign Out") }
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks identical to the if block apart from the text "Refresh".

OH. I see what this is doing now. Maybe factor out the "Clear Storage..." button and refer to it twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

mm, that is probably better. done


<p>{ _t("If you have previously used a more recent version of Riot, your session " +
"may be incompatible with this version. Close this window and return " +
"to the more recent version.") }</p>

{ bugreport }
<p>{ _t("Clearing your browser's storage may fix the problem, but will sign you " +
"out and cause any encrypted chat history to become unreadable.") }</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation could be better here

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Apr 30, 2018
…-org/matrix-react-sdk into dbkr/bug_report_dialog_basedialog
…alog_ux' into dbkr/bug_report_dialog_basedialog
@lukebarnard1 lukebarnard1 merged commit 80c67ba into develop Apr 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants