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

adds context to new list item button #472

Merged
merged 2 commits into from Jul 4, 2017

Conversation

DrianHillman
Copy link
Contributor

Fixes Issue #300

- Summary
This PR adds the list label to the new button which provides users better context (especially with nested lists).

- Test plan
All tests continue to pass. I created nested lists 20 levels deep to test & ensure scalability. I ran npm build test & npm run line in compliance with contributing.md guidelines.

Example screenshot of the result here: Screenshot


- Description for the changelog

  • Adds the specific content type to the list control widget's "new" button.

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

Basketball Dog :)

@erquhart
Copy link
Contributor

erquhart commented Jul 3, 2017

@DrianHillman thanks for the fast PR! I added one change request, should be good to merge after that.

@DrianHillman
Copy link
Contributor Author

Sure thing, @erquhart! I'd be happy to take a look at a revision, however, I don't think it posted. Can you clarify what it is?

@@ -171,11 +171,13 @@ export default class ListControl extends Component {

renderListControl() {
const { value, forID } = this.props;
const listLabel = this.props.field._root.entries[0][1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Two pointers:

  • The field prop, along with most state collections in the CMS, are using Immutable data structures. In this case it's a Map, so you'll want to use get to access it's properties. Also note that you can more easily inspect/debug Immutable collections by using the toJS method, which converts the collection to a plain object or array. Again, this is just for inspection purposes.
  • You can shorten things a tad by adding field to the object deconstruction in the previous line.

Applying these points, you get:

const { value, forId, field } = this.props;
const listLabel = field.get('label');

@erquhart
Copy link
Contributor

erquhart commented Jul 3, 2017

Sorry about that, forgot to post. It's up now.

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 once @erquhart's change request is addressed.

@DrianHillman
Copy link
Contributor Author

  • Edits confirmed!

Learned a new technique today— thanks @erquhart! I appreciate the breakdown & links; this makes sense. The Immutable package looks great, it makes the access to props & data more performant and much more concise. 👍🏽

@erquhart
Copy link
Contributor

erquhart commented Jul 4, 2017

@DrianHillman awesome work, man. If you're available/interested, join us on Wednesday for our first bi-weekly community planning session via Hangouts: https://www.eventbrite.com/e/netlify-cms-planning-session-bi-weekly-tickets-35794059997

@erquhart erquhart merged commit cdf7c3a into decaporg:master Jul 4, 2017
@DrianHillman
Copy link
Contributor Author

@erquhart Thanks! Wish I could make it tomorrow, but I'm all tied up at that time. It is a great idea to meet regularly about the project and stay on the same page with contributors— that's very cool, I'll keep an eye out for future ones.

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

3 participants