Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

New APIs for footer/header view manipulation #9

Merged
merged 4 commits into from
Oct 18, 2016

Conversation

zplesac
Copy link
Member

@zplesac zplesac commented Oct 18, 2016

Defined new methods for footer/header view manipulation.

* @return true if footer was added/replaced, false otherwise.
*/
public boolean addFooter(@LayoutRes int footerViewId, boolean shouldReplace) {
if (!hasFooter() || hasFooter() && shouldReplace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would reverse this logic to make it a bit cleaner.

if (shouldReplace || !hasFooter()) {
}

Copy link
Member Author

Choose a reason for hiding this comment

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

By reversing the logic - you always have to set shouldReplace value to true. If we stick to the current solution - you can try to add the footer with shouldReplace set to false and it can silently fail if we already have footer view.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this? If you set shouldReplace to false this condition will move to !hasFooter() and it will silently fail if footer view is already added. If you set shouldReplace to true, it will never check if footer already exists.

Maybe there is a use-case which I am missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've discussed about this on Slack, you are right.

* @return true if footer was added/replaced, false otherwise.
*/
public boolean addFooter(View footerView, boolean shouldReplace) {
if (!hasFooter() || hasFooter() && shouldReplace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as the comment for addFooter(@LayoutRes int footerViewId, boolean shouldReplace) method

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

* @return true if header was added/replaced, false otherwise.
*/
public boolean addHeader(@LayoutRes int headerViewId, boolean shouldReplace) {
if (!hasHeader() || hasHeader() && shouldReplace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

* @return true if header was added/replaced, false otherwise.
*/
public boolean addHeader(View headerView, boolean shouldReplace) {
if (!hasHeader() || hasHeader() && shouldReplace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

@zplesac zplesac merged commit bc4f661 into master Oct 18, 2016
@zplesac zplesac deleted the feature/header-updates branch October 18, 2016 13:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants