Uploading preview adjustments #241
Conversation
|
@flamerohr, thanks for your PR! By analyzing the blame information on this pull request, I identified @chillu, @clarkepaul and @sminnee to be potential reviewers |
flamerohr
force-pushed
the
pulls/4.0/upload-preview
branch
2 times, most recently
from
3a31447 to
29677b1
Compare
| @@ -154,13 +154,17 @@ class GalleryItem extends SilverStripeComponent { | |||
| return this.props.item.exists; | |||
| } | |||
|
|
|||
| uploading() { | |||
There was a problem hiding this comment.
Masking a prop in a trivial getter seems unnecessary here - it just makes the code harder to read. What's the rationale for this?
There was a problem hiding this comment.
The intent was that the exists() function was already in place and I was originally tweaking that function instead.
Turned out it needed splitting out and I had found it easier to read this.uploading() alongside this.exists() as opposed to mixing this.props.uploading with this.exists()
Feedback on commit message: You can leave out the URL if its on the same repo, so this should be:
Also, careful with line length on commit message first lines - it should be less than 100 chars or so, otherwise most git repo browsers (like github) will cut it |
chillu
force-pushed
the
pulls/4.0/upload-preview
branch
from
d119c05 to
27ef3be
Compare
|
Rebased on master and force pushed. |
fixes #221
Also fixes:
handleAddedFilereturns a promise, so it can be chained for unit testing (and if the caller wants to chain something else)Found new issue
Gallery.handleCancelUpload()is broken