Update delete shot confirmation dialog. #4341
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4341 +/- ##
==========================================
+ Coverage 45.81% 45.93% +0.12%
==========================================
Files 36 36
Lines 1600 1600
Branches 289 288 -1
==========================================
+ Hits 733 735 +2
+ Misses 867 865 -2
Continue to review full report at Codecov.
|
| @@ -261,6 +261,14 @@ shotIndexPageNextPage = | |||
| shotIndexNoExpirationSymbol = ∞ | |||
| .title = This shot does not expire | |||
|
|
|||
|
|
|||
| ## Delete Confirmation Dialog | |||
There was a problem hiding this comment.
nit: leave a blank line after a section comment
|
@flodolo Heh, those title attributes sounded pretty awkward to me when I was reviewing the commit. I'll change them to simply match the button text. |
chenba
force-pushed
the
4186-delete-confirmation
branch
from
78c325e to
70d9d6a
Compare
static/css/frame.scss
Outdated
| @@ -51,6 +52,10 @@ | |||
| margin-right: 4px; | |||
| } | |||
|
|
|||
| button.trash.shot-delete-confirmation { | |||
There was a problem hiding this comment.
Two thoughts here: first, it seems odd that you press the button, giving it the 'active' highlight, then it changes to the lighter 'hover' highlight. Maybe using the 'active' color would be better.
Also, since this class just applies a 'hover' (er, 'active') effect, it might make more sense to make this a more generic 'active' (or 'active-button') class.
| border: 1px solid #c7c7c7 ; | ||
| border-radius: 3px; | ||
| box-shadow: 0 5px 11px 0 rgba(0, 0, 0, 0.25), 0 0 0 0 rgba(0, 0, 0, 0.29), inset 0 0 0 0 #fff; | ||
| height: 114px; |
There was a problem hiding this comment.
Right now, if the string in the box grows (always possible in German), the buttons are pushed outside the dialog container:
If you change this line from a height to a min-height, the container should grow to fit the content.
| } | ||
| } | ||
| .delete-confirmation-message { | ||
| font-size: 12px; |
There was a problem hiding this comment.
The text size seems unusually small, compared to other body / detail text in the shot page header:
Maybe 15px would be better?
There was a problem hiding this comment.
hmm I thought I grabbed the font-size straight out of https://sevaan.github.io/fxscreenshots/menus/#artboard0, but it's 13px there (which is still small). @sevaan what do you think of 15px?
There was a problem hiding this comment.
You're right, that does seem small. 15px would work great (matching the font size that says "2 minutes ago" in your screenshot above.
server/src/pages/shot/view.js
Outdated
| } | ||
|
|
||
| componentDidUpdate() { | ||
| if (this.state.confirmDelete) { |
There was a problem hiding this comment.
I kinda think it would make more sense for the state to track whether the delete button is pressed, matching the panelOpen state. This is pretty minor, though.
Edit: this is just an optional suggestion
server/src/pages/shot/view.js
Outdated
| maybeCloseDeleteConfirmation(e) { | ||
| let el = e.target; | ||
|
|
||
| while (el) { |
There was a problem hiding this comment.
I suspect you could use Node.contains() to simplify this, avoiding the while loop
There was a problem hiding this comment.
True. Just need to get refs to the dialog and the delete button.
There was a problem hiding this comment.
Yeah. If this is getting extracted to its own component, maybe pass the delete button ref into the constructor: "dialog, attach to this element here"
server/src/pages/shotindex/view.js
Outdated
| @@ -293,6 +293,19 @@ class Card extends React.Component { | |||
| constructor(props) { | |||
| super(props) | |||
| this.state = {panelOpen: "panel-closed", deleted: false}; | |||
| this.maybeCloseDeleteConfirmation = this.maybeCloseDeleteConfirmation.bind(this); | |||
There was a problem hiding this comment.
There is a lot of code duplication between these two view files. Could you extract the modal code to a single shared component?
There was a problem hiding this comment.
Yeah, that code duplication is pretty gross. I'll try styling up a div as a button and have the confirmation dialog in there. (Currently the dialog is actually a sibling of the button...which is the main reason they aren't in a component.)
static/css/shot-index.scss
Outdated
| @@ -252,6 +258,17 @@ h1 { | |||
| } | |||
| } | |||
|
|
|||
| .delete-confirmation-dialog { | |||
There was a problem hiding this comment.
Maybe the default dialog style could be declared in one spot, and overridden in the other, to minimize duplication? Not sure if it's worth it, your call
There was a problem hiding this comment.
Since the dialog is absolute positioned to the delete button's parent, the defaults that work for one page would need to be completely overridden for the other page anyway, so I left them out of the partial.
chenba
force-pushed
the
4186-delete-confirmation
branch
from
70d9d6a to
eb6d850
Compare
chenba
dismissed
jaredhirsch’s stale review
Moved the markup and the confirmation panel state into a component.
… (mozilla-services#4341)" This reverts commit 324fb38.
Fixes #4186