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

Disable add #1102

Merged
merged 9 commits into from
Mar 15, 2018
Merged

Disable add #1102

merged 9 commits into from
Mar 15, 2018

Conversation

gazebosx3
Copy link
Contributor

- Summary
Closes issue #1065
Adds option to disable "add new item" in list widgets via flag in config.yml

- Test plan
Button disappears if disableAdd flag is marked true

code_false
with_button
code_true
no_button

- Description for the changelog

Add disableAdd flag for list widget in config.yml

- A picture of a cute animal (not mandatory but encouraged)

suki

@verythorough
Copy link
Contributor

verythorough commented Feb 12, 2018

Deploy preview for cms-demo ready!

Built with commit 943c4aa

https://deploy-preview-1102--cms-demo.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Feb 12, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 943c4aa

https://deploy-preview-1102--netlify-cms-www.netlify.com

@gazebosx3
Copy link
Contributor Author

Hey, just checking in on this PR--is there a problem with my changes? If so, happy to rework it.

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.

@gazebosx3 apologies for the wait! Thanks for this, looks great. Just a couple of small change requests and it should be good to go.

Add {listLabel} <Icon type="add" size="xsmall" />
</button>
:
<div />
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use null here instead of an empty div.

const SortableList = SortableContainer(({ items, renderItem }) => {
return <div>{items.map(renderItem)}</div>;
});
const SortableList = SortableContainer(({ items, renderItem }) => <div>{items.map(renderItem)}</div>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting should stay as it was here - we try to keep line length at or below 100 characters, hence the line break.

We should really get Prettier in place to automatically handle this stuff.

@@ -27,6 +27,7 @@ collections: # A list of collections the CMS should be able to edit
- name: notifications
label: Notifications
widget: list
disableAdd: false
Copy link
Contributor

Choose a reason for hiding this comment

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

We're generally phrasing boolean flags in the positive using snake_case, so rather than disableAdd: true, it would be more like allow_add: false.

@gazebosx3
Copy link
Contributor Author

Sorry for the delay, and changes in! It's allow_add in the .yml but still camel case in the component (got a message from the linter as such); let me know if I should change that or anything else.

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.

Just noticed we're not defaulting to true, added a couple of comments around that.

@@ -258,6 +264,7 @@ export default class ListControl extends Component {
return (
<div id={forID} className={c(classNameWrapper, 'nc-listControl')}>
<TopBar
allowAdd={field.get('allow_add')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't realize we weren't setting this to true by default - we'll need to do so, otherwise this is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so should it be more like allowAdd={field.get('allow_add' = true)}?

Copy link
Contributor

@erquhart erquhart Feb 28, 2018

Choose a reason for hiding this comment

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

Close: field.get('allow_add', true)

Check the Immutable.Map.get docs for details.

@@ -27,6 +27,7 @@ collections: # A list of collections the CMS should be able to edit
- name: notifications
label: Notifications
widget: list
allow_add: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be necessary with allow_add defaulting to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this flag should just be removed from config.yml? If so, how would it be editable by users?

Copy link
Contributor

Choose a reason for hiding this comment

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

They would only need to add it if setting the value to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gazebosx3 This is just our copy of the CMS for editing the website, it doesn't affect what users can do at all.

Copy link
Contributor

@verythorough verythorough left a comment

Choose a reason for hiding this comment

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

@gazebosx3 Thanks for adding this functionality! Could you update the docs so people will know it exists? 😄

You'll find the file here: https://github.com/netlify/netlify-cms/blob/master/website/site/content/docs/widgets/list.md

I'd add it under options, probably between default and field.

@gazebosx3
Copy link
Contributor Author

Documentation updated with definition/example!

@erquhart
Copy link
Contributor

@verythorough look good to you?

@gazebosx3 there's still some merge conflict pointers in the README, can you fix?

@erquhart
Copy link
Contributor

Thanks @gazebosx3!

@erquhart erquhart merged commit a2d4267 into decaporg:master Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants