Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extend and lock current GUI support to just YAML and JSON data files #360

Merged
merged 12 commits into from
May 3, 2017
Merged

Extend and lock current GUI support to just YAML and JSON data files #360

merged 12 commits into from
May 3, 2017

Conversation

ashmaroli
Copy link
Member

improves GUI for data files and tests with all Jekyll-supported data files

@ashmaroli
add multiple data file types
3a8e2a4
@ashmaroli
support proper syntax highlighting for JSON data in Editor
74569dd 
Currently JSON data is also displayed with "yaml" mode in the Editor.
With this commit, JSON data will be displayed under "json" mode while
every other data file type (YAML, CSV, TSV) will have "yaml" as the mode.
@ashmaroli
extend GUI Editor support to JSON data files
9e01cb0
@ashmaroli
lock GUI Editor Support to only YAML and JSON data files
2eb933a
@ashmaroli
fix css bug
e6c2dc0
@ashmaroli
fix tests
db56d16
mertkahyaoglu
mertkahyaoglu requested changes May 2, 2017
View reviewed changes
src/actions/datafiles.js Outdated
@@ -38,7 +38,7 @@ export function fetchDataFile(filename) {
};
}

export function putDataFile(filename, data, source = "editor") {
export function putDataFile(filename, data, source = "editor", ext = "") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not get the extension from the filename? We have a helper for that too.

src/actions/datafiles.js Outdated
@@ -51,8 +51,11 @@ export function putDataFile(filename, data, source = "editor") {
payload = { raw_content: data };
} else if (source == "gui") {
const metadata = getState().metadata.metadata;
const yaml_string = toYAML(metadata);
payload = { raw_content: yaml_string };
if (ext == ".yml" || ext == ".yaml") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use regex here to include uppercase ones as well.

const yaml = /yaml|yml/i.test(ext);

src/components/Editor.js Outdated
@@ -21,11 +22,12 @@ class Editor extends Component {
}

render() {
const { content } = this.props;
const { content, ext } = this.props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make ext prop type without dot.

src/components/Editor.js Outdated
@@ -21,11 +22,12 @@ class Editor extends Component {
}

render() {
const { content } = this.props;
const { content, ext } = this.props;
const mode = (ext == ".json") ? "json" : "yaml";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex again.

src/containers/views/DataFileEdit.js Outdated

// Prevent the default event from bubbling
preventDefault(e);

if (fieldChanged) {
putDataFile(params.data_file, null, 'gui');
putDataFile(params.data_file, null, 'gui', ext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again we can retrieve ext from params.data_file.

src/containers/views/DataFileEdit.js Outdated
const filename = getFilenameFromPath(path);

const guiSupport = (ext == ".yml" || ext == ".yaml" || ext == ".json") ? true : false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex again.

src/containers/views/DataFileEdit.js Outdated
@@ -78,9 +79,9 @@ export class DataFileEdit extends Component {

renderAside() {
const { datafile, datafileChanged, onDataFileChanged, fieldChanged, updated } = this.props;
const { path } = datafile;
const { path, ext } = datafile;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be retrieved from datafile.

src/containers/views/DataFileEdit.js Outdated
@@ -120,7 +124,7 @@ export class DataFileEdit extends Component {
return <h1>{getNotFoundMessage("data file")}</h1>;
}

const { path, raw_content, content } = datafile;
const { path, raw_content, content, ext } = datafile;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already have filename below this line. For this and above comments, I think it would be better to have a getExtension helper.

src/containers/views/DataFileEdit.js Outdated
<div className="content-body">
<Editor
editorChanged={datafileChanged}
onEditorChange={onDataFileChanged}
content={raw_content}
ext={ext}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type for this too.

spec/fixtures/site/_data/members.tsv
@@ -0,0 +1,3 @@
name age sex location
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think csv will do the job. Let's remove this 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

csv and tsv (supported by Jekyll 3.5 and beyond) have been included here for better futureproof

@ashmaroli
Copy link
Member Author

@mertkahyaoglu Thanks for the wonderful suggestions 👍

mertkahyaoglu
mertkahyaoglu reviewed May 3, 2017
View reviewed changes
src/utils/helpers.js Outdated
if (!path) return '';
return path.substring(path.lastIndexOf('.') + 1);
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in 1fcc613 .. changed the function a bit to account for paths that do not contain an extension

@ashmaroli
Copy link
Member Author

ashmaroli commented May 3, 2017 •
edited

I'm not able to run coveralls or script/cibuild-node successfully locally on Windows. The run hangs at babel-node tools/testCi.js

Update: I'm able to run test:cover as expected though..

mertkahyaoglu
mertkahyaoglu reviewed May 3, 2017
View reviewed changes
src/utils/helpers.js Outdated
return path.substring(path.lastIndexOf('.') + 1);
const index = path.lastIndexOf('.');

if (index >= 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be =? I think last . at the beginning is not a valid path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dotFiles are valid filepaths.. just not in current context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Since we are checking the last . here, dotFiles with no extension will get into if statement. For example, .babelrc; it has no extension but index variable will be 0 and returned extension will be babelrc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a test for dotFiles as well.

@mertkahyaoglu
Copy link
Member

Thanks @ashmaroli 👍

@mertkahyaoglu mertkahyaoglu merged commit 6e4c87b into jekyll:master May 3, 2017
@ashmaroli ashmaroli deleted the gui-plus branch May 12, 2017 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertkahyaoglu mertkahyaoglu mertkahyaoglu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ashmaroli @mertkahyaoglu