Media Library #554

erquhart · 2017-08-25T20:51:26Z

The media library UI is feature complete. Here are the remaining steps:

remove temporary "media" widget and convert image/field widgets to use media library
implement support for asset provider integrations
add support for in-memory test backend
ensure asset filenames are lowercased
add media library support for markdown editor
migrate netlify-auth support to git-gateway (rebase to 0.5.0-beta branch)

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.

dopry · 2017-09-12T00:24:26Z

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.

There is a bug with uploads, see dopry@37456b1

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.

Redundant steps when adding an existing image to an entry
1. click +
2. click image (Insert Image dialog pops up)
3. click in dialog to open media library (media library dialog pops up)
4. select image in media library
5. click insert (media library dialog closes)
6. click insert (Insert Image dialog closes)
Note that steps 2,3 and 5,6 are redundant.
You have to specifically check the box to insert media. Its a small hit target, it might be better if clicking the row selected the checkbox.
it would be really cool if just dropping an image on the text editor would upload a piece of content, and insert an image box.
can't just paste a URL in the image widget.

erquhart · 2017-09-27T15:40:57Z

@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.

erquhart · 2017-10-20T22:58:17Z

@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.

Benaiah · 2017-10-20T23:00:40Z

@erquhart 🙌 awesome! I'll get to reviewing.

tech4him1

There seems to be a regression here related to the routing changes -- I can no longer create a new post.

tech4him1 · 2017-10-21T17:36:24Z

src/containers/EntryPage.js

  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;


This should be reverted, this is breaking new entry creation.

Fixed - thanks for finding that.

erquhart · 2017-11-01T03:01:09Z

ALRIGHT ITS REALLY READY FOR REAL NOW GUYS

erquhart · 2017-11-03T03:41:12Z

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.

Benaiah

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.

Benaiah · 2017-11-07T01:33:40Z

package.json

@@ -124,8 +125,10 @@
    "webpack-postcss-tools": "^1.1.1"
  },
  "dependencies": {
+    "bytes": "^2.5.0",


It doesn't look like this dependency is being used in the final diff - should this be removed?

Yep, table view leftover, good catch.

Benaiah · 2017-11-07T01:43:12Z

src/backends/github/implementation.js

+      return { id: response.sha, name: value, size: fileObj.size, url, path: trimStart(path, '/') };
+    }
+    catch(error) {
+      console.error(error);


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?

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.

@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.

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.

Benaiah · 2017-11-07T02:04:25Z

src/backends/github/API.js

+         * 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);


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);.

Beautiful, done.

Benaiah · 2017-11-07T02:06:20Z

src/backends/github/API.js

+       * 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`.


🎉 💯 great way to leverage Git immutability to detect API weirdness.

Benaiah · 2017-11-07T02:18:13Z

src/backends/github/API.js

+       * 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;


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.

Yep, I'm with that.

Benaiah · 2017-11-07T02:29:12Z

src/components/MediaLibrary/MediaLibrary.js

+  /**
+   * Upload a file.
+   */
+  handlePersist = async event => {


Didn't know you could use async with a naked argument in an arrow function - cool!

tech4him1 · 2017-11-07T02:44:37Z

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.

erquhart · 2017-11-07T21:03:13Z

@tech4him1 I've been thinking of, at some point, creating and uploading thumbnails to the metadata branch for this purpose, definitely agree.

tech4him1

A little inconsistent naming here -- was it intentional?

tech4him1 · 2017-11-08T02:11:47Z

src/components/Widgets/FileControl.js

        <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'}


Here we call it the asset library.

tech4him1 · 2017-11-08T02:12:08Z

src/components/Widgets/ImageControl.js

        <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'}


Here we call it the image library.

erquhart · 2017-11-08T02:35:32Z

@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.

tech4him1 · 2017-11-08T02:38:27Z

@erquhart Awesome, that's what I was hoping, just wanted to check.

talves

When adding a media file with the same name, it appears twice in the ui.

talves · 2017-11-08T06:08:48Z

@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.

erquhart · 2017-11-08T15:39:31Z

@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?

talves · 2017-11-08T15:48:17Z

#783

* 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

erquhart · 2017-11-08T18:46:24Z

Yarn is having issues, going to hold off on merging until their resolved.

hacknug · 2017-11-08T19:17:52Z

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 Click here to select an image from the media library and Click here to select an asset from the media library instead of repeating the filtered keyword.

I think even something like Click here to select an image from the library would be better since "select an image" already implies you're allowing the user to only pick image files from the media library.

erquhart · 2017-11-08T20:00:22Z

@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.

erquhart changed the title ~~WIP - Add Media Library~~ WIP - Media Library Aug 25, 2017

erquhart force-pushed the media-library-ui branch 5 times, most recently from b30abd7 to 6727267 Compare August 31, 2017 02:40

tech4him1 mentioned this pull request Aug 31, 2017

WIP: Gentle re-authentication modal/popup. #560

Closed

erquhart force-pushed the media-library-ui branch 4 times, most recently from 91effe5 to fde75b9 Compare September 8, 2017 04:44

erquhart force-pushed the media-library-ui branch 3 times, most recently from e54e371 to a7444e3 Compare September 19, 2017 13:52

erquhart force-pushed the media-library-ui branch 3 times, most recently from 646c1d5 to 1542f42 Compare October 10, 2017 14:59

erquhart force-pushed the media-library-ui branch 2 times, most recently from 5fa530e to fb6e36d Compare October 20, 2017 22:19

erquhart changed the title ~~WIP - Media Library~~ Media Library Oct 20, 2017

erquhart force-pushed the media-library-ui branch from 44a38b4 to 409b9b4 Compare October 20, 2017 22:57

erquhart requested review from talves, Benaiah and tech4him1 October 20, 2017 22:58

tech4him1 suggested changes Oct 21, 2017

View reviewed changes

erquhart force-pushed the media-library-ui branch 3 times, most recently from 11ead33 to 8a091e1 Compare November 3, 2017 03:37

erquhart force-pushed the media-library-ui branch from 3302df5 to 431bfd2 Compare November 3, 2017 14:40

tech4him1 self-requested a review November 3, 2017 15:16

Benaiah approved these changes Nov 7, 2017

View reviewed changes

tech4him1 approved these changes Nov 7, 2017

View reviewed changes

tech4him1 self-requested a review November 7, 2017 02:39

erquhart force-pushed the media-library-ui branch 3 times, most recently from 4dd1bf7 to 52906d9 Compare November 7, 2017 21:33

tech4him1 reviewed Nov 8, 2017

View reviewed changes

erquhart force-pushed the media-library-ui branch from 52906d9 to 7306baf Compare November 8, 2017 02:39

talves reviewed Nov 8, 2017

View reviewed changes

add media library

35f2e8c

* 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

erquhart force-pushed the media-library-ui branch from 7306baf to 35f2e8c Compare November 8, 2017 18:24

erquhart merged commit 6b45a46 into master Nov 8, 2017

erquhart deleted the media-library-ui branch November 8, 2017 20:00

Media Library #554

Media Library #554

Conversation

erquhart commented Aug 25, 2017 • edited Loading

dopry commented Sep 12, 2017 • edited Loading

erquhart commented Sep 27, 2017

erquhart commented Oct 20, 2017

Benaiah commented Oct 20, 2017

tech4him1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Nov 1, 2017

erquhart commented Nov 3, 2017 • edited Loading

Benaiah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

erquhart Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Benaiah Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Nov 7, 2017

erquhart commented Nov 7, 2017

tech4him1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Nov 8, 2017

tech4him1 commented Nov 8, 2017

talves left a comment

Choose a reason for hiding this comment

talves commented Nov 8, 2017

erquhart commented Nov 8, 2017

talves commented Nov 8, 2017

erquhart commented Nov 8, 2017

hacknug commented Nov 8, 2017

erquhart commented Nov 8, 2017

erquhart commented Aug 25, 2017 •

edited

Loading

dopry commented Sep 12, 2017 •

edited

Loading

erquhart commented Nov 3, 2017 •

edited

Loading

Benaiah Nov 7, 2017 •

edited

Loading

Benaiah Nov 7, 2017 •

edited

Loading

erquhart Nov 8, 2017 •

edited

Loading

Benaiah Nov 7, 2017 •

edited

Loading

Benaiah Nov 7, 2017 •

edited

Loading

Benaiah Nov 7, 2017 •

edited

Loading