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

Add yuidoc blocks to pager-control.js, see #223 #633

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

pixelhandler
Copy link
Contributor

What's in this PR?

  • Added yuidoc blocks in app/components/pager-control.js file

Progress on: #223

Copy link
Contributor

@rileytaylor rileytaylor left a comment

Choose a reason for hiding this comment

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

@joshsmith
Copy link
Contributor

@rileytaylor @pixelhandler it's a good question as to what extent we're overdocumenting in other cases and if this is the right level of documentation here. Are some of these functions self-documenting enough just by naming convention? I'm not totally positive, but am certainly open to some competing ideas here to see what direction we should take.

@rileytaylor
Copy link
Contributor

@joshsmith @pixelhandler agreed. Some of the simple boolean values could go without description, but some could definitely use @default tags. Also the beginning header describing the entire component.

@pixelhandler
Copy link
Contributor Author

@joshsmith

Are some of these functions self-documenting enough just by naming convention?

If you plan to use generated docs, only methods with doc blocks will show up. For example here are the generated docs I use for an addon project, http://ember-jsonapi-resources.com/docs/

@pixelhandler
Copy link
Contributor Author

@rileytaylor I'll add the defaults and the component doc block as well, thanks

@joshsmith
Copy link
Contributor

@pixelhandler I wasn't clear: I meant that you didn't need much more than @property canShowPages, for example, given the self-documenting nature.

@pixelhandler
Copy link
Contributor Author

@joshsmith cool thanks for the clarity, I do like seeing @type w/ @property

@joshsmith
Copy link
Contributor

I was unclear again! I just meant that you didn't need extra English-language description about what it means.

@pixelhandler
Copy link
Contributor Author

pixelhandler commented Oct 23, 2016

@joshsmith with generated docs the English language description can be handy even when the property name is super targeted, there is usually some clarity that can be added. Also the generated docs too read nice with human descriptions vs variable names only, here is another example page of props, http://ember-jsonapi-resources.com/docs/classes/ApplicationAdapter.html

@joshsmith
Copy link
Contributor

Okay, then I would definitely consider adding those descriptions here, then.

@pixelhandler
Copy link
Contributor Author

@rileytaylor @joshsmith I updated this PR after rebasing on develop, Added doc block for the component as well as human friendly descriptions for the properties.

Copy link
Contributor

@WenInCode WenInCode left a comment

Choose a reason for hiding this comment

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

I've reviewed the previous feedback, and checked out the latest changes. This LGTM thanks @pixelhandler

Feel free to squash & merge

@joshsmith
Copy link
Contributor

👍 please do rebase and squash when you can @pixelhandler!

@pixelhandler
Copy link
Contributor Author

pixelhandler commented Oct 25, 2016

@joshsmith I rebased and squashed; so CI should run again :)

- Add component block and human readable descriptions
@pixelhandler
Copy link
Contributor Author

@rileytaylor do you need to 👍 for the requested changes ?

@rileytaylor
Copy link
Contributor

yes, looks like I do! Looks good now.

@joshsmith joshsmith merged commit 7de75d4 into code-corps:develop Oct 25, 2016
@joshsmith
Copy link
Contributor

🙌

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.

4 participants