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 YARD documentation the PersonCard class #15614

Merged
merged 2 commits into from
Feb 23, 2017
Merged

Conversation

chrismytton
Copy link
Contributor

What does this do?

Adds YARD documentation to the PersonCard class.

Why was this needed?

We want to document all the classes in this app as part of #15310.

More specifically I want to properly document the new methods I'm adding for cabinet memberships, so documenting the rest of this class first means I'm not changing the whole world at once when adding docs for that.

Relevant Issue(s)

Part of #15310

Should make #15606 easier to document…

Screenshots

person-card-yard

We want to eventually document all classes in the app, so this is
another step in that direction.
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15614 February 23, 2017 14:21 Inactive
@chrismytton chrismytton requested a review from tmtmtmtm February 23, 2017 14:26
Copy link
Contributor

@tmtmtmtm tmtmtmtm left a comment

Choose a reason for hiding this comment

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

Mostly good, other than a slightly confusing use of 'current'.

def identifiers
Section::Identifiers.new(person, top_identifiers: top_identifiers).data
end

# List of memberships this person holds in the current term
Copy link
Contributor

Choose a reason for hiding this comment

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

current is a little too ambiguous here, I fear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I had the same thought while doing 5000246! Have fixed that in fc67a72.

The use of the word "current" was a bit misleading in this context, it
could mean either the current page this class is being used on or the
current term that's in session right now.
@tmtmtmtm tmtmtmtm merged commit 4c6ed28 into master Feb 23, 2017
@tmtmtmtm tmtmtmtm removed the 3 - WIP label Feb 23, 2017
@chrismytton chrismytton removed their assignment Feb 23, 2017
@tmtmtmtm tmtmtmtm deleted the person-card-yard-docs branch December 16, 2017 16:57
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.

2 participants