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.

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

boolean widget & some documentation #396

Merged
merged 2 commits into from May 18, 2017
Merged

Conversation

daddykotex
Copy link
Contributor

@daddykotex daddykotex commented May 2, 2017

- Summary

While working on the file-system backend, I was working with hugo. hugo uses a draft attribute to let you mark content as draft so it's not included in the output. I found there was no boolean widget, so here it is.

The MR passes npm run lint:staged but I have issues the the pre-commit hooks:

.git/hooks/pre-commit: line 2: ./node_modules/pre-commit/hook: No such file or directory

- Test plan

There's no test for the other widget, so I did not really know where to start here. Let me know if you want one.

screen shot 2017-05-01 at 21 41 41

- Description for the changelog
Widget for boolean fields

@erquhart
Copy link
Contributor

erquhart commented May 2, 2017

@daddykotex great addition! Also love the bonus docs improvement, thanks for that.

Two thoughts:

  1. Are you up for improving this to use the React Toolbox switch widget? The dependency is already there, and it's a very simple setup.
  2. I realize other widgets such as the Number widget are using classes for no reason, but we want to use functional components wherever possible moving forward.

@daddykotex
Copy link
Contributor Author

Something like:

screen shot 2017-05-02 at 22 20 27

One thing I'm unsure is the behavior if value is undefined, like if required: false and draft is not in the front matter for example. Right now, I think there's a stack in the console.

@Benaiah
Copy link
Contributor

Benaiah commented May 3, 2017

@daddykotex I think the best way to handle this would be to require a default in the widget declaration in config.yml, and then have missing or undefined values default to that.

@biilmann
Copy link
Contributor

biilmann commented May 3, 2017

screen shot 2017-05-03 at 2 14 20 pm

There's still some UX issues with some of the widgets

@Benaiah
Copy link
Contributor

Benaiah commented May 3, 2017

@biilmann did you mean to make that comment on @rafaelconde's PR? I'm not following how it's related to this one.

@daddykotex
Copy link
Contributor Author

@Benaiah that's a good idea, I'll play around to see how to implement that ;)

@daddykotex
Copy link
Contributor Author

Ok, I turned it functionnal (AFAIK 😄 ), it uses the switch widget and falls back to the default value or false if not supplied.

But now, I can't save because the validation proccess in ControlHOC.js seems to use refs and react gives undefined as wrappedControl in the function processInnerControlRef.

It blows up on save because validation is triggered and const response = this.wrappedControlValid(); is called and at this point, it's undefined for the boolean controller.

Do you have any idea?

@daddykotex daddykotex force-pushed the bool branch 2 times, most recently from 536060e to 61df8f4 Compare May 9, 2017 23:45
@daddykotex
Copy link
Contributor Author

Any input here?

@daddykotex
Copy link
Contributor Author

This implementation works correctly: daddykotex@eb1538c

@Benaiah
Copy link
Contributor

Benaiah commented May 10, 2017

@daddykotex that commit does appear to be working the first time you edit a post, but afterwards I can't open any entry for editing. I'm looking into the issue.

@daddykotex
Copy link
Contributor Author

OK, so I'll try to be really clear so we avoid wasting time due to confusion.

When I use the class implementation (bool-class branch https://github.com/daddykotex/netlify-cms/tree/bool-class). It works fine. I can save the boolean value, I can have a default value as well.

When I use the fully functional approach (bool branch https://github.com/daddykotex/netlify-cms/tree/bool), it does not work properly. I can't save and this is probably due to what I explained above (related to ref and ControlHOC). I see the following stack on save:

screen shot 2017-05-13 at 09 43 53

By the way, I rebased both branch on master and I updated the example/config.yml to have a draft using the boolean widget. This way, it might be wasier to test for you.


On another note, I see that on FF and Chrome, the included example has a slightly different behavior. On Firefox, the markdown widget has a switch turned on while it's off on chrome:

screen shot 2017-05-13 at 09 36 10

I think this is completely unrelated though.

@erquhart
Copy link
Contributor

@daddykotex functional components don't have refs, so class is fine.

@daddykotex
Copy link
Contributor Author

Ok, so the implementation now uses a class.

Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

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

@daddykotex My bad - the bug that I mentioned earlier (#420) was actually unrelated to this PR, and is now fixed. I've tested this again after rebasing onto the latest master and it works fine. 👍 to merge as soon as the config line is removed.

@@ -15,6 +15,7 @@ collections: # A list of collections the CMS should be able to edit
- {label: "Publish Date", name: "date", widget: "datetime", format: "YYYY-MM-DD hh:mma"}
- {label: "Cover Image", name: "image", widget: "image", required: false, tagname: ""}
- {label: "Body", name: "body", widget: "markdown"}
- {label: "Draft", name: "draft", widget: "boolean", required: false, defaultValue: true}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to use the example config as a test case for new features - can this line be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

@daddykotex
Copy link
Contributor Author

removed the line and rebase against master :)

@erquhart
Copy link
Contributor

Simplified just a bit.

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

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

LGTM

@Benaiah Benaiah merged commit aac5339 into decaporg:master May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants