Support editing data files in sub-directories #349

ashmaroli · 2017-04-21T15:48:34Z

Fixes #302

Update API to support data files in sub-dirs
Update Front End
Update documentation

API Breaking Changes

path, relative_path, absolute_path is now derived from data file id
'path' != 'relative_path'
relative_path is now relative to data_dir and does not have a leading /
file id now has a .yml extension even if the assoc. file doesn't.

read from the `id` attribute for path generation - redefine `relative_path` from the `data_file_id` by stripping away `data_dir` in the file id. - redefine `absolute_path` here to include subdirectories based on `splat` params. - point `path` to the data file `id` instead of returning the same data from `relative_path` as previously. - consistent `resource_path`

- hrefs for data file folders require a trailing slash to differentiate from data files without an extension. - separate `datafiles` to render `Data Files` instead of `Datafiles`

ashmaroli · 2017-04-22T10:26:05Z

Front-end Changes

CRUD data files recursively
Added ability to rename data files. Rename their path to move a file from one directory to another.

ashmaroli · 2017-04-22T14:32:56Z

@benbalter This is ready for your review.
The relative_path now outputs path relative to the data_dir and it doesn't have a leading / to easily join with directory paths that have a trailing / to differentiate itself from a path to an id without an extension.

mertkahyaoglu · 2017-04-24T15:11:28Z

src/components/Breadcrumbs.js

-          href: `${base}/${before}${path}`,
-          label: path
-        };
+        if (type == "data files") {


Can't we include the trailing slash in path variable like we do with other types?

are you referring to the pre-API variable or path within Breadcrumbs? If its the latter, the reason is that variable before uses path to generate its value and I get the following:

Data Files / movies/ / genres/ /

Trailing slash should be optional for the API. For example while /_api/data/movies/ returns a result , /_api/data/movies doesn't.

Actually, /_api/data/movies/ returns result for the /movies/ folder and /_api/data/movies returns for movies.yml file, both being valid data source in their own context and both resulting in site.data.movies

But we defined our routes so that individuals are called with their extensions.

get "/*?/?:path.:ext"

/_api/data/movies returning for movies.yml file doesn't make sense for me. IMO we need to be as specific as possible while calling the endpoints.

/_api/data/movies returning for movies.yml file doesn't make sense for me.

It didn't make sense to me too till I read the spec files.

I'm suggesting to fix it since we are proposing sub-directory support now.
@benbalter What do you think?

But we defined our routes so that individuals are called with their extensions.

Its not so according to master

get "/:path.?:ext?" do

(I vote for the more sensible resolution too)

Yeah, I just noticed that :face_palm:

I think it was a flexible solution at the time, props to @benbalter 🎉 Directories change everything 😄

mertkahyaoglu · 2017-04-24T15:12:00Z

src/components/Breadcrumbs.js

@@ -12,7 +12,7 @@ export default class Breadcrumbs extends Component {
    let base;
    if (type == 'pages') {
      base = `${ADMIN_PREFIX}/pages`;
-    } else if (type == 'datafiles') {
+    } else if (type == 'data files') {


Why rename?

I had included the explanation for this in the commit message:

[...] to render into 'Data Files' instead of 'Datafiles'

mertkahyaoglu · 2017-04-24T15:16:48Z

src/containers/views/DataFileEdit.js

@@ -47,23 +63,36 @@ export class DataFileEdit extends Component {
    }
  }

+  getDirectoryfromPath(path) {


You can retrieve the directory via;

const [directory, ...rest] = params.splat;

yes I've used that in a few places elsewhere.

mertkahyaoglu · 2017-04-24T18:06:26Z

src/components/form/InputPath.js

-    }else if (type == 'datafiles') {
-      placeholder = 'your-filename.yml';
+    }else if (type == 'data files') {
+      placeholder = '<directory/>your-filename.yml';


Typo here => <directory> / your-filename.yml

actually, it was intended to show that directory and / are optional. The / needed only if there was a directory param.

(directory/)?filename.yml (directory/)filename.yml [directory/]filename.yml <directory/>filename.yml

I didn't get it. I don't think non-technical users will do 😬 HTML tag came to my mind directly.

Ah! I see. I'll settle for the your suggestion then <directory> / your-filename.yml

mertkahyaoglu · 2017-04-24T18:20:50Z

src/containers/views/DataFileEdit.js

+            <InputPath
+              onChange={onDataFileChanged}
+              type="data files"
+              path={path} // TODO: Support using `relative_path` from API instead


For pages and documents, only filenames are passed to InputPath component. Relative paths are shown in the Breadcrumb component. We should be consistent with them. I'm testing locally and seeing /_data/data_file.yml instead for InputPath. Before the filename is already shown to users in the Breadcrumbs component. No need to repeat it. Let's abstract that.

/_data/data_file.yml is shown because that parameter (path) is required to be changed to rename the data file.

renaming /_data/data_file.yml to /_data/foo/data_file.yml will move the data file into a subdirectory named foo.

Yeah, that's expected. What I'm saying is just show data_file.yml like posts, for example. You can rename it to foo/data_file.yml and it will drop into /_data/foo/data_file.yml automagically 😄 Just check out other types, you will understand the difference.

I did check other types. Hence added the //TODO here for a future PR

That info is already exposed via path, relative_path and api_url. We just need to parse it.

On master, path == relative_path for this resource.
In this PR, they are redefined.
So, do we parse it like below ?

const data_dir = path - relative_path

--
update: strings cannot be subtracted only concatenated

Yeah, that should work.

what do you think about enabling the tooltip for DataFileEdit? If yes, what should the message be?

I can't think of one.

mertkahyaoglu · 2017-05-12T18:32:01Z

how would you move _data/movies/genres/fiction.yml to _data/books/favs/fiction.yml via the DataFileEdit container?

I intentionally disabled this functionality when I was implementing directory support for Pages and Collections. I don't think it's a good idea to give users too much flexibility/power in the front-end. They can mistakenly move a file to another directory or maybe another content type or even out of Jekyll site and it is really troublesome to move it back.

IMO, these type of operations should be done via OS, not via the GUI. And also I don't want to bring extra complexity to the codebase. This feature requires a bit too much validation to deal with for now. In the future, when Jekyll Admin can be hosted on the cloud, we may implement this or maybe drag-and-drop functionality for files.

Finishing off by quoting from Ben Balter: Sometimes removing/omitting a feature is a feature ☯️

ashmaroli · 2017-05-12T19:32:25Z

Alrite then, I'll make the change.. funny, that field's label says Path: when it should rightly be Filename..

Regarding the other concerns, directory traversal is already delimited by the API. The put action will always be within JekyllAdmin::Datafile.data_dir, and other content-types are possible even at current master by simply changing the extension in the InputPath. That guard has been provided by the DataGUI though..

Sometimes removing/omitting a feature is a feature

Cool quote! But it doesn't send a clear answer: yes or no

mertkahyaoglu · 2017-05-12T19:40:05Z

funny, that field's label says Path: when it should rightly be Filename

Nope, you can go deeper from the current directory by passing test/othertest/page.md to InputPath, for example, but you can't go to the opposite direction. Therefore Path makes sense, I think..

ashmaroli · 2017-05-13T06:29:48Z

@mertkahyaoglu addressed the two things that bothered you.. 😄

mertkahyaoglu · 2017-05-13T09:34:36Z

lib/jekyll-admin/server/data.rb

-      end
-
-      get "/:path.?:ext?" do
+      get "/*?/?:path.?:ext?" do


Let's go with /*?/?:path.:ext 👍

so to confirm, we're dropping support for accessing extension-less data files?

k.. so change frontend to send requests to /data/foo-dir or leave it at /data/foo-dir/?

mertkahyaoglu · 2017-05-13T09:34:58Z

lib/jekyll-admin/server/data.rb

@@ -23,7 +24,7 @@ class Server < Sinatra::Base
        json written_file.to_api(:include_content => true)
      end

-      delete "/:path.?:ext?" do
+      delete "/*?/?:path.?:ext?" do


…file

mertkahyaoglu · 2017-05-13T14:11:43Z

src/containers/views/DataFileNew.js

@@ -117,7 +117,7 @@ export class DataFileNew extends Component {
      datafileChanged, fieldChanged, onDataFileChanged, datafile, updated, errors, params
    } = this.props;
    const { path, raw_content } = datafile;
-    const initialPath = params.splat ? (params.splat + "/") : "";
+    const initialPath = params.splat ? (params.splat) : "";


This is easier.

const initialPath = params.splat || '';

Actually this is not used at all.

mertkahyaoglu · 2017-05-13T14:17:11Z

src/containers/views/DataFiles.js

@@ -30,14 +30,13 @@ export class DataFiles extends Component {
  handleClickDelete(path) {
    const { deleteDataFile, params } = this.props;
    const confirm = window.confirm(getDeleteMessage(path));
-    const directory = params.splat ? (params.splat.replace(/\/$/, "")) : "";
-    const goTo = params.splat ? (params.splat) : "";
+    const directory = params.splat ? (params.splat) : "";


Again.

const directory = params.splat || '';

Ah yes.. its a remnant from an earlier commit. One cant call replace on a nil entity..
will change..

mertkahyaoglu · 2017-05-13T19:55:44Z

src/containers/views/DataFiles.js

+    const { deleteDataFile, params } = this.props;
+    const confirm = window.confirm(getDeleteMessage(path));
+    const directory = params.splat || "";
+    const dir = directory ? `/${directory}` : ""


Semicolon here 😄

aargh.. I hate it that the linter doesn't report this while running npm run test:watch

mertkahyaoglu · 2017-05-13T20:02:23Z

@ashmaroli What do you think about type converting? Let's say I created a JSON data file first, then decided to switch to YAML using the selectbox. Currently it's not converting. Shall we support it or expect users to decide first then input their data accordingly? 😄

mertkahyaoglu · 2017-05-13T20:05:49Z

src/containers/views/DataFileEdit.js

  }

  handleClickSave(e) {
    const { datafile, datafileChanged, fieldChanged, putDataFile, params } = this.props;
+    const { path, relative_path } = datafile;
+    const data_dir = path.replace(relative_path, "");


Try to use single quote for convention please. I also do that 😄

the convention I saw used in all container/views/* is that single quotes are used at the top when importing components, and double quotes within the current component class body.
So I think its best to handle all those files in a separate dedicated PR..

Yeah. I also forget them sometimes 😬 Just a reminder for future commitments. Appreciate for the PR though 👍

Let me know if that PR will get merged.. I can then correct files here as part of resolving conflicts

I was planning to merge it last.

okay then..

ashmaroli · 2017-05-14T02:33:09Z

What do you think about type converting? Let's say I created a JSON data file first, then decided to switch to YAML using the selectbox. Currently it's not converting. Shall we support it..

I had added support for it via the DataGUI.. but checking now, it seems like minor refactors I made to the putDatafile action across various PRs introduced an uncaught bug where the type conversion only takes place when altering the basename and ext for the said file. I'll fix it ~~and extend support to the raw editor as well..~~

UPDATE: Raw Editor won't handle data type conversion as its output is WYSIWYG

mertkahyaoglu · 2017-05-14T10:42:11Z

@ashmaroli One last thing. Could you move Create button above Switch button for DataFileNew?

mertkahyaoglu

Thanks @ashmaroli . This one is huge 😄

ashmaroli · 2017-05-14T12:04:49Z

Sorry for the long list of commits 😇
Thank you @mertkahyaoglu for the time and patient review 😃

mertkahyaoglu · 2017-05-14T12:06:46Z

@ashmaroli Glad to work with you 😄

Should've hit squash and merge 😬

ashmaroli added 5 commits April 21, 2017 21:06

redefine DataFile.all to match files recursively

da0ef4d

extend dataFilesAPI to support CRUD on data files recursively

6a4788d

store extension within data file id

c398a79

update specs

e4f02c9

ashmaroli force-pushed the data-directories branch from 8f761b0 to e4f02c9 Compare April 21, 2017 15:54

ashmaroli added 8 commits April 22, 2017 14:50

test if API supports renaming data files

9f0b72e

export updated API to frontend

c7134f1

update frontend routes

30b5973

update datafiles actions

a1f4d35

update Breadcrumbs and InputPath

0a5f8d3

- hrefs for data file folders require a trailing slash to differentiate from data files without an extension. - separate `datafiles` to render `Data Files` instead of `Datafiles`

update DataFile containers

73ca5c4

update test files

bc2986a

update API to exclude directories that do not contain data files

7550848

ashmaroli added 3 commits April 22, 2017 16:08

appease travis?

2d88157

Update documentation

8537583

appease Travis

4955489

Merge branch 'master' into 'data-directories'

972e3e8

mertkahyaoglu reviewed Apr 24, 2017

View reviewed changes

mertkahyaoglu requested changes Apr 24, 2017

View reviewed changes

ashmaroli added 2 commits April 24, 2017 22:49

getDirectory from splats parameter

68987bb

Merge branch 'master' into data-directories

7d762af

ashmaroli force-pushed the data-directories branch from 2c61786 to 7d762af Compare April 24, 2017 17:21

mertkahyaoglu reviewed Apr 24, 2017

View reviewed changes

ashmaroli added 2 commits April 25, 2017 19:57

edit placeholder

fb733c1

use relative_path in InputPath for DataFileEdit

8b572f2

ashmaroli added 2 commits May 13, 2017 11:26

limit directory traversal to deeper levels

7a8d1d1

modify params.splat in Datafiles view based on context

e95fb2b

fix tests

deaca7e

mertkahyaoglu added this to the v0.5.0 milestone May 13, 2017

mertkahyaoglu reviewed May 13, 2017

View reviewed changes

ashmaroli added 2 commits May 13, 2017 16:21

remove API support to access data files without extensions as a YAML …

ec0033f

…file

update front end views

cc062a4

mertkahyaoglu reviewed May 13, 2017

View reviewed changes

minor fixes

5f9f0ce

mertkahyaoglu reviewed May 13, 2017

View reviewed changes

ashmaroli added 3 commits May 14, 2017 08:55

Merge branch 'master' into data-directories

22518b7

fix data type conversion via DataGUI

3bab4ff

add missing semicolon

624002d

ashmaroli added 2 commits May 14, 2017 17:18

Merge branch 'master' into data-directories

b1d2aac

have the toggleButton and saveButton switch places

4624c58

mertkahyaoglu approved these changes May 14, 2017

View reviewed changes

mertkahyaoglu merged commit 3813da4 into jekyll:master May 14, 2017

ashmaroli deleted the data-directories branch May 14, 2017 12:11

Support editing data files in sub-directories #349

Support editing data files in sub-directories #349

Conversation

ashmaroli commented Apr 21, 2017 • edited

API Breaking Changes

ashmaroli commented Apr 22, 2017

Front-end Changes

ashmaroli commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertkahyaoglu Apr 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmaroli Apr 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertkahyaoglu commented May 12, 2017

ashmaroli commented May 12, 2017

mertkahyaoglu commented May 12, 2017 • edited

ashmaroli commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertkahyaoglu May 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertkahyaoglu commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmaroli commented May 14, 2017 • edited

mertkahyaoglu commented May 14, 2017

mertkahyaoglu left a comment

Choose a reason for hiding this comment

ashmaroli commented May 14, 2017

mertkahyaoglu commented May 14, 2017

ashmaroli commented Apr 21, 2017 •

edited

mertkahyaoglu Apr 24, 2017 •

edited

ashmaroli Apr 25, 2017 •

edited

mertkahyaoglu commented May 12, 2017 •

edited

mertkahyaoglu May 13, 2017 •

edited

ashmaroli commented May 14, 2017 •

edited