-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Disable add #1102
Conversation
Deploy preview for cms-demo ready! Built with commit 943c4aa |
Deploy preview for netlify-cms-www ready! Built with commit 943c4aa |
Hey, just checking in on this PR--is there a problem with my changes? If so, happy to rework it. |
There was a problem hiding this 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 /> |
There was a problem hiding this comment.
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>); |
There was a problem hiding this comment.
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.
website/site/static/admin/config.yml
Outdated
@@ -27,6 +27,7 @@ collections: # A list of collections the CMS should be able to edit | |||
- name: notifications | |||
label: Notifications | |||
widget: list | |||
disableAdd: false |
There was a problem hiding this comment.
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
.
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. |
There was a problem hiding this 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')} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)}?
There was a problem hiding this comment.
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.
website/site/static/admin/config.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
.
Documentation updated with definition/example! |
@verythorough look good to you? @gazebosx3 there's still some merge conflict pointers in the README, can you fix? |
Thanks @gazebosx3! |
- 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
- Description for the changelog
Add disableAdd flag for list widget in config.yml
- A picture of a cute animal (not mandatory but encouraged)