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

use StringRes for setTitles() in the builder #41

Merged
merged 1 commit into from
Feb 13, 2017
Merged

use StringRes for setTitles() in the builder #41

merged 1 commit into from
Feb 13, 2017

Conversation

hardysim
Copy link
Contributor

@hardysim hardysim commented Feb 6, 2017

fixes #30

@hardysim
Copy link
Contributor Author

hardysim commented Feb 6, 2017

I changed the signature of MultiChoiceToolbar.Builder#setTitles() to (String, String) and (@StringRes int, @PluralsRes int) for the ease of implementation.

For the user, he can choose (int, int) which should be the default way to use it or (String, String) when the titles needs to be made up on-the-fly.

@dvdciri
Copy link
Owner

dvdciri commented Feb 6, 2017

Ok Good, just increase the MINOR version of the release. I'll merge it.

Did you test it with the sample app? Is everything ok? backwards-compatible?

@hardysim
Copy link
Contributor Author

hardysim commented Feb 7, 2017

backwards-compatible?

It was not because I've removed the setTitles(String, @PluralsRes int) in favor of setTitles(@StringRes int, @PluralsRes int) but I reverted it nowso we're fully backwards-compatible.

I've added a @Deprecated to it as well. All samples already use the new (int, int)-style so any user will have see the indented way of using it (and not the deprecated version).

@dvdciri
Copy link
Owner

dvdciri commented Feb 7, 2017

Wait, you shouldn't merge everything into this branch you should rebase onto develop, in order to rewrite the history and not end up having 14 commits not needed! Maybe at this point, create another PR from updated develop, cherry pick just the commits that you want for this PR, and if you fancy squash them into one. Otherwise i'll do it.

@hardysim
Copy link
Contributor Author

hardysim commented Feb 7, 2017

Hmm, I did it after realizing that the changelog isn't up-to-date with the master. And as develop is not meant to be behind master I merged both 😉 (a reason for #42).

So the problem seems to be my merge of master into my feature-branch. A merge of develop should not result in any unwanted commits, does it?


I did a rebase and squashed my 2 commits. Should be fine now...

@hardysim
Copy link
Contributor Author

hardysim commented Feb 7, 2017

Sry for the mess with the merge-commits (should be ok when merge + squash). The last one comes from syncing my branches with yours. My tree looks good now and I'm prepared for the future 😄.

@hardysim
Copy link
Contributor Author

hardysim commented Feb 8, 2017

@dvdciri can you have a look (and merge)?

@dvdciri
Copy link
Owner

dvdciri commented Feb 8, 2017

There are still 7 commits here.. can you please squash everything in one commit that includes CHANGELOG, config bump, library and sample changes?
Thanks

@hardysim
Copy link
Contributor Author

hardysim commented Feb 8, 2017

can you please squash everything in one commit

You said you can do a sqash+merge when merging. This would be far easier then me screwing up an other squash...

@dvdciri
Copy link
Owner

dvdciri commented Feb 9, 2017

I can squash into one commit but i can't change the message and rebase it. So please use the UI in SourceTree for doing an interactive rebase onto the last commit of develop and quash everything into a single commit adding a proper commit message and i'll be happy to merge.

@hardysim
Copy link
Contributor Author

hardysim commented Feb 9, 2017

I can squash into one commit but i can't change the message

so you'll never do a squash-merge because the message would be "too ugly"?


rebased and good to go now

@hardysim
Copy link
Contributor Author

@dvdciri any updates on this?

@dvdciri dvdciri merged commit f1467a2 into dvdciri:develop Feb 13, 2017
@hardysim hardysim deleted the 30-string-res-title branch February 13, 2017 15:19
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