feat(media dir nav): navigation through sub directories and breadcrumbs

[andrew-paterson]

This is just a small first step in attempting nested media directories (#3240)

So far it only works with Github, and only navigation up and down directories with breadcrumbs is implemented. As I'm not too familiar with the codebase, it would help to have someone give it a cursory sanity check, before continuing too far on the current path.

My approach has been to add a listDirs function to packages/netlify-cms-backend-github/src/API.ts and then update the getMedia() function in packages/netlify-cms-backend-github/src/implementation.tsx to run both listFiles() and listDirs() and concatenate the result.

This means both the directories and files are passed to the MediaLibrary component in the files array. As the Github type is included with each object in the array, the toTableData() function in packages/netlify-cms-core/src/components/MediaLibrary/MediaLibrary.js sets the isDirectory prop for each file object, based on whether it is a tree or blob. The MediaLibraryCard component can display and behave accordingly.

I'm struggling to see if it would be wiser to pass the directories to the component as a separate array altogether. One issue is that a directory should not technically have a displayURL prop, but if ommitted the following error is thown:

Warning: Failed prop type: The prop files[0].displayURL is marked as required in MediaLibrary, but its value is undefined

I'm also not 100% sure if the way in which I've added the updateMediaFolder() action is correct. Lastly, in order to get the default media directory into the breadcrumbs component, I have ended up adding an action called getDefaultMediaFolder() to set a prop called defaultMediaFolder and pass that to the component.

I'm guessing there is an easier way to get a prop from the site config into a component?

[erezrokah]

Hi @andrew-paterson, thank you for this - great draft PR.
I believe the code can be simplified as we can retrieve all of the nested files and directories in a single request by passing a recursive flag to GitHub (see depth: 100 argument to listFiles), then doing the filtering on the client side.

Then we only need to maintain the currentFolder state in the UI component.

See my commit to reflect those changes and please let me know what you think.

I haven't look into the UI yet, but I hope my changes address your questions.

I think the next steps are to see how we handle file uploads and also editorial workflow. For the latter each entry can have its own media folder at the global level, collection level and even at the field level.
After that we would need to add support for the other backends.

We could also build the directories in the UI instead of retrieving them based on the files paths

[andrew-paterson]

• Uploads to nested directories are working

  • As I can't get the currentMediaFolder from the app config, I am passing it to the selectMediaFolder function in packages/netlify-cms-core/src/reducers/entries.ts as an argument. I'm not sure if there is a better way to do this.
  • I have updated the selectMediaFolder function to simply return the currentMediaFolder if it is present, thus ignoring any config settings.

• Selecting and deleting multiple assets is implemented

  • Folders with children cannot be selected. What about making this behaviour configurable? I would probably prefer to be able to delete a folder and its contents recursively as an end user.
  • I'm using await to delete batches of files in sequence. I tried doing it asynchronously first, but the second commit request errored.

• I have begun updating some styling to match the spec in Improve The Overall UX #2557. I pretty quickly reached a point where it was clear that updating some global styling settings would be necessary to go much further in doing this. I just wanted to check in about this before starting to overhaul the guts of what is there. If that's fine, I'm happy to take on Improve The Overall UX #2557 as part of this.

• Is there a preferred place to put utility functions like readableFileSize which I have currently left in packages/netlify-cms-core/src/components/MediaLibrary/MediaLibraryCard.js? I would like that to be available in other components, and I thought I'd ask if any conventions exist on where to put it.

• For custom media_folder at the collection level, I need to get the custom folder setting into the MediaLibrary component, in order to filter correctly here:

  const currentMediaFolder = this.state.currentMediaFolder || defaultMediaFolder;
  const currentDirFiles = files.filter(file => dirname(file.path) === currentMediaFolder);
  

  where the above could be something like:

  const customMediaFolder = ***Not sure how to get this for a collection or field***
  const currentMediaFolder = this.state.currentMediaFolder || customMediaFolder || defaultMediaFolder;
  const currentDirFiles = files.filter(file => dirname(file.path) === currentMediaFolder);
  

• I also need a bit more time to understand the editorial workflow, as I have not used that yet, and so I don't really know how it will affect the media browser.

• I added max as a size to packages/netlify-cms-ui-default/src/Icon.js which sets the width and height to 100%, so you can have SVG icons that fill the available space.

• In terms of the next steps, would the ability to upload multiple files at once appeal? I think it would be good to show a UI with a select button and a drag and drop, and then show the list of queued files and update the UI for each one as it starts and finishes uploading.

[mildred]

Thank you very much for you work, I started using Netlify a short while ago and I am still in the process of publishing my first website managed this way, and the first issue that came was that there is no hierarchy in the media folder (the site is composed of many photo galleries). I just discovered that you are working on this. And indeed, the next thing is going to be the file upload where uploading a single file at a time is not very convenient.

[andrew-paterson]

@erezrokah thank you for the refactor! A few updates:

• I have implemented a new way of deriving the current media folder in cases where the media library has loaded for the first time, and the user has not yet navigated the directory structure. Instead of falling back to the default media folder from config, which is incorrect for collections or fields, I derive it from the file paths.

• I have also implemented sub-directory navigation when the media library is defined on a collection or field, and have added a config setting for disable_media_folder_navigation which prevents this if true. Please consider this experimental, and feel free to change it. As an end-user, I would love to retain the ability to limit the media browser to one directory for a specific field.

• The media library component sets state.currentMediaFolder to null whenever the modal is closed. This prevents a user from being in their last location when they close it for one field and open it for another.

I think the next things should be folder creation and uploading multiple files, but I will wait for your feedback before I start on any of that.

[andrew-paterson]

@erezrokah thank you. I have pushed an update where inserting files from a field seems to be working better now, both where a field has a media_folder setting and where it does not (In this case, the navigation is enabled). Things are looking much better now after a lot of manual testing.
I will look into the automated tests next.

[andrew-paterson]

Running tests for http://localhost:8080/__/#/tests/integration/media_library_spec_github_backend_rest.js, I get a failure when trying to save the entry. API 404 seems to be an error from the mock server. Is this correct? I just want to make sure I understand where this error is coming from.

Incidentally, saving an entry with an image using the editorial workflow works fine when I do it manually, pointing to my own Github repo.

[erezrokah]

API 404 seems to be an error from the mock server. Is this correct? I just want to make sure I understand where this error is coming from.

Yes the server is returning 404 for a specific request. We use pre-recorded data, so something might have changed with the requests the CMS sends.

I would first make sure the non GitHub/Bitbucket/GitLab/Git Gateway tests work as they don't rely on pre-recorded data.

[andrew-paterson]

Ok all of the following tests are passing:

editorial_workflow_spec_test_backend.js
field_validations_spec.js
i18n_editorial_workflow_spec_test_backend.js
markdown_widget_backspace_spec.js
markdown_widget_code_block_spec.js
markdown_widget_enter_spec.js
markdown_widget_hotkeys_spec.js
markdown_widget_link_spec.js
markdown_widget_list_spec.js
markdown_widget_marks_spec.js
markdown_widget_quote_spec.js
media_library_spec_test_backend.js
search_suggestion_spec.js
simple_workflow_spec_test_backend.js
view_filters_spec.js
view_groups_spec.js

I believe those are all the e2e tests that don't rely on a mock backend service. Any ideas on how to approach the backend specific tests? I have no idea what is going wrong with those.

[erezrokah]

I believe those are all the e2e tests that don't rely on a mock backend service. Any ideas on how to approach the backend specific tests? I have no idea what is going wrong with those.

Let me look into the Cypress failing tests. Does yarn test pass on your machine (these are unit tests)? It's failing on CI at the moment.

[andrew-paterson]

The result of yarn test. Seems like there are linting errors in two files if I'm reading that correctly?

~/Documents/development/netlify-cms [feat/nested_media_dirs] $ yarn test
yarn run v1.9.4
$ run-s clean lint type-check test:unit
$ rimraf "packages/*/dist" dev-test/dist
$ run-p -c --aggregate-output "lint:*"
$ stylelint --ignore-path .gitignore "{packages/**/*.{css,js,jsx,ts,tsx},website/**/*.css}"
Build Package: netlify-cms
error Command failed with exit code 1.
$ prettier "{{packages,scripts,website}/**/,}*.{js,jsx,ts,tsx,css}" --list-different
packages/netlify-cms-core/src/actions/mediaLibrary.ts
packages/netlify-cms-core/src/reducers/entries.ts
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
$ eslint --color --ignore-path .gitignore "{{packages,scripts,website}/**/,}*.{js,jsx,ts,tsx}"
Build Package: netlify-cms
ERROR: "lint:format" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "lint" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[erezrokah]

You can run yarn format to fix the formatting issues, then run yarn test again

[andrew-paterson]

Thanks. I need to do that and push one more update.

[andrew-paterson]

Ok there are 4 unit tests failing, that I'm struggling with a little- notes below.

The same e2e tests mentioned above are all passing.

1

entries › selectMediaFilePublicPath › should resolve path from public folder for collection with no media folder

The selectMediaFilePublicPath now removed the media_folder from the beginning of the path and replaces it with the public_folder, so that the full path of the image be maintained. It was previously just joining the public folder and the filename.

This test only passes the public folder to the function:

selectMediaFilePublicPath(
  { public_folder: '/media' },
  null,
  '/media/image.png',
  undefined,
  undefined,
),

I don't think this is allowed by the actual application, and I'm not sure I can get this test to pass with the new pattern.

2

FAIL  packages/netlify-cms-core/src/actions/__tests__/mediaLibrary.spec.js  
  ● mediaLibrary › persistMedia › should not persist media when editing draft

FAIL  packages/netlify-cms-core/src/reducers/__tests__/mediaLibrary.spec.js
  ● mediaLibrary › should select draft media files from field when editing a draft
  ● mediaLibrary › should select draft media files from collection when editing a draft

In the latest version of the selectMediaFiles function, if (editingDraft && !integration) is true, I get the enry files, and concatenate them with mediaLibrary.get('files'), as you need all the files to allow directory navigation when the media library is opened from a field. Previously this function would only return the entry files.

It's working fine for me in the app, but in the unit tests, mediaLibrary.get('files') is returning an empty list:

List {
  size: 0,
  _origin: 0,
  _capacity: 0,
  _level: 5,
  _root: undefined,
  _tail: undefined,
  __ownerID: undefined,
  __hash: undefined,
  __altered: false
}

After concatenating the media files and entry files, I don't get any objects, and this is causing issues.

I'm not sure if this is just an issue with the tests themselves, or if this is a problem for the wider application?

[SignSpice]

Curious what the status is on this pull request. I am very interested in this feature.

[SignSpice]

@erezrokah What still needs to be done for this to be merged? Is it only the two merge conflicts and the 3 (I think) failing tests?

[erezrokah]

@erezrokah What still needs to be done for this to be merged? Is it only the two merge conflicts and the 3 (I think) failing tests?

Hi @SignSpice

1. Fix merge conflicts
2. Look into existing tests failures and verify it doesn't break current behavior (specifically nested collections, global media folder, collection level media folder and field level media folder)
3. Add support for all backends
4. Add new tests (e.g Cypress)
5. More thorough code review

[martinjagodic]

@andrew-paterson are you still interested in moving this forward?