-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Media Library #554
Media Library #554
Conversation
b30abd7
to
6727267
Compare
91effe5
to
fde75b9
Compare
I've got a rebased version of this at https://github.com/dopry/netlify-cms/tree/media-library-ui, you can grab it or cherry pick from there as you need. I'd send a PR, but my branch is based on master and won't merge.
The workflow for inserting images is a little convoluted, not sure if there is a good way to streamline, but figured I'd write up some of my notes. some of it we may want to hoist up into #350, unless its something easily fixed in the PR.
|
e54e371
to
a7444e3
Compare
@dopry just saw your feedback, great stuff! I'll look into the upload bug and selection by clicking rows, the rest is probably going to happen under the UI refresh as the redundancies are bigger than just the media library. |
646c1d5
to
1542f42
Compare
5fa530e
to
fb6e36d
Compare
44a38b4
to
409b9b4
Compare
@Benaiah @tech4him1 @talves this is ready to rock. I went ahead and squashed, too many commits to be super useful anyway. I left the temporary live backend in the example config for review purposes, we'll need to revert that before merging. |
@erquhart 🙌 awesome! I'll get to reviewing. |
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.
There seems to be a regression here related to the routing changes -- I can no longer create a new post.
src/containers/EntryPage.js
Outdated
const slug = ownProps.match.params.slug; | ||
const collection = collections.get(ownProps.match.params.name); | ||
const newEntry = ownProps.newRecord === true; | ||
const newEntry = ownProps.match.newRecord === true; |
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 should be reverted, this is breaking new entry creation.
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.
Fixed - thanks for finding that.
ALRIGHT ITS REALLY READY FOR REAL NOW GUYS |
11ead33
to
8a091e1
Compare
Note: ignore code style issues unless they're catastrophic. Didn't have the luxury of doing it clean, but improvements will probably come with the UI work prior to 1.0. Gotta get this puppy released. The original work is still the first commit, the rest is split into separate commits to ease review for anyone that already went over that whopping first commit. If you haven't reviewed that first commit, it might be simpler to just review on file changes, since a fair amount from the original commit gets changed/removed. |
3302df5
to
431bfd2
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.
There's a few stylistic decisions that break the linter, but in most of those cases the lint rule is unnecessary or wrong IMO (there's a lot of "don't use a naked single argument for an arrow function with braces" which I think is silly), and you specifically stated we're de-emphasizing style on this PR (:+1: from me on that). Just noting here that this doesn't pass our linter, which IMO we should change by either fixing the linter or moving to prettier
(at some point).
99% of this LGTM - great job. There's a couple things that seem a little weird to me, but nothing major and I haven't noticed any regressions. I'll approve to allow merge, since I think it's fine to do so to get this out the door, but I do have comments for @erquhart below.
package.json
Outdated
@@ -124,8 +125,10 @@ | |||
"webpack-postcss-tools": "^1.1.1" | |||
}, | |||
"dependencies": { | |||
"bytes": "^2.5.0", |
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.
It doesn't look like this dependency is being used in the final diff - should this be removed?
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.
Yep, table view leftover, good catch.
return { id: response.sha, name: value, size: fileObj.size, url, path: trimStart(path, '/') }; | ||
} | ||
catch(error) { | ||
console.error(error); |
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.
It seems inappropriate to handle and log this error here, which prevents it from going up the Promise chain and being handled in a user-friendly manner. Is this try...catch
block just for debugging?
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.
Need to re-throw - try
/catch
is the new then
/catch
, since async await is written in a synchronous manner. If we just let the error go we'll lose the stack, an issue that we can maybe mitigate if we overhaul error handling at some point. The approach for now is to log the stack trace and then re-throw so it travels up, as you mentioned.
Will fix.
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.
@erquhart maybe I'm wrong, but my understanding is that uncaught exceptions in an async function return a promise rejection rather than erroring synchronously. Because of this, a try...catch
that just rethrows the error is redundant IIUC.
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.
It's totally redundant, but it let's us get the stack trace at the source. I haven't found a better way, at least with the way we handle errors now (haphazardly), to ensure we get an accurate trace.
Each instance of this pattern came out of me troubleshooting something and not getting the stack trace I needed with an error, didn't do it across the board. Moving forward we definitely need to solidify our approach to error handling, along with migrating over to async/await in general.
src/backends/github/API.js
Outdated
* Use `baseCommit` as the parent of the first commit, and normalize | ||
* commit data to ensure it's not nested in `commit.commit`. | ||
*/ | ||
const parent = this.normalizeCommit(idx === 0 ? baseCommit : newParent); |
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 ternary could be eliminated by changing the base value of the reduce
to Promise.resolve(baseCommit)
and then changing this line to const parent = this.normalizeCommit(newParent);
.
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.
Beautiful, done.
src/backends/github/API.js
Outdated
* Sometimes the list of commits for a pull request isn't updated | ||
* immediately after the PR branch is patched. If the branch is known to | ||
* have been updated very recently, `assertHead` will be appended, but | ||
* only if the last commit in the response is the parent of `assertHead`. |
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.
🎉 💯 great way to leverage Git immutability to detect API weirdness.
src/backends/github/API.js
Outdated
* have been updated very recently, `assertHead` will be appended, but | ||
* only if the last commit in the response is the parent of `assertHead`. | ||
*/ | ||
const missingHead = assertHead && assertHead.parents[0].sha === last(prCommits).sha; |
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.
assertHead
implies to me that the function will error if I pass assertHead
and neither it nor its parent(s) matches the most recent SHA. Instead, getPullRequestCommits
silently continues, which seems prone to mishap - if the repo moves out from under us we probably don't want to blindly continue our mutations. OTOH, I can see people using assertHead
without making mutations, who don't want an error here. A good fix IMO would be to separate this into two different methods - getPullRequestCommits
(with no assertHead
parameter) and assertHead
(which does the conditional concatenation, and can have its errors caught and handled separately from API or HTTP errors) - which would allow the caller to decide how to handle it on a case-by-case basis, and simplify the related functions. (the equivalent to what this does now would be getPullRequestCommits(prNumber).then(commits => assertHead(commits, head).catch(() => commits))
, which IMO expresses the intent much more clearly, eliminates a confusing argument from getPullRequestCommits
, and makes what should be the most common desired behavior of assertHead
(erroring when mutating a repo which has state we didn't expect) the default.
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.
Yep, I'm with that.
/** | ||
* Upload a file. | ||
*/ | ||
handlePersist = async event => { |
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.
Didn't know you could use async with a naked argument in an arrow function - cool!
This might be for a second-level (after merge) fix, but I wish there was a way so that we didn't have to download the entire full-resolution images when viewing the media library, or at least download them async. If you had a large media library, this could really bog down your computer. |
@tech4him1 I've been thinking of, at some point, creating and uploading thumbnails to the metadata branch for this purpose, definitely agree. |
4dd1bf7
to
52906d9
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.
A little inconsistent naming here -- was it intentional?
<span className="nc-fileControl-message" onClick={this.handleClick}> | ||
{fileName ? fileName : 'Click here to upload a file from your computer, or drag and drop a file directly into this box'} | ||
{fileName ? fileName : 'Click here to select an asset from the asset library'} |
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.
Here we call it the asset library.
<span className="nc-fileControl-message" onClick={this.handleClick}> | ||
{imageName ? imageName : 'Click here to upload an image from your computer, or drag and drop a file directly into this box'} | ||
{fileName ? fileName : 'Click here to select an image from the image library'} |
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.
Here we call it the image library.
@tech4him1 the media library can show all assets, or just images. The image control uses this to ensure non-images aren't available for selection in the image widget, and the messaging is tweaked accordingly. |
@erquhart Awesome, that's what I was hoping, just wanted to check. |
52906d9
to
7306baf
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.
@erquhart I was testing out the local api against this code to make the necessary method calls, when I saw the above. So, I tested on the preview deploy against the pr-554-backend and saw the same happen. |
@talves good catch - we're faking it to reduce API calls, but we need a way to check for this. Can you open a separate issue for it? |
* rebase editorial workflow pull requests when behind * fix async/await transpilation * add media library pagination * switch media library to grid layout * ensure that only cms branches can be force updated
7306baf
to
35f2e8c
Compare
Yarn is having issues, going to hold off on merging until their resolved. |
Talking about the naming inconsistencies you were discussing earlier, if this PR title is "Media Library" (and most instances in the code use that same term) why not go with I think even something like |
@hacknug yeah, we need to have more considered verbiage around this, agreed. I'm about to merge this (finally!!), but if you open a separate issue we can discuss from there. |
The media library UI is feature complete. Here are the remaining steps:
Note that the media library will be in place for the editorial workflow, but will not be subject to the editorial workflow process.
Closes #350.
Closes #494.
Closes #229.
Closes #247.