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

Unification for activities start #137

Merged
merged 3 commits into from
Oct 14, 2019

Conversation

vbuberen
Copy link
Collaborator

Minor set of changes to make a unified way of how TransactionActivity and ErrorActivity start, since after recent merges these operations looked quite different.

Copy link
Member

@olivierperez olivierperez left a comment

Choose a reason for hiding this comment

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

Just some standardization of constants.

private const val EXTRA_THROWABLE_ID = "EXTRA_THROWABLE_ID"
private const val TEXT_PLAIN = "text/plain"
Copy link
Member

Choose a reason for hiding this comment

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

Can you move all the constants to the companion? To standardize it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I just thought that classes had such constants declaration, because you prefer to avoid companion objects when possible, like some devs do (for example: https://medium.com/@BladeCoder/exploring-kotlins-hidden-costs-part-1-fbb9935d9b62 ).
As to this particular constant for TEXT_PLAIN - I wanted to remove it in another PR, but for now will just move it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link 👍


private const val ARG_TRANSACTION_ID = "transaction_id"
private var selectedTabPosition = 0
Copy link
Member

Choose a reason for hiding this comment

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

Oh! I just found something... interesting 👀
I guess the previous developer used selectedTabPosition as a static in order to store in memory. And used it when view needs to re-open (in setupViewPager).

For this PR, could you let selectedTabPosition in the companion? I'm not fan of the idea, but for now I just want to merge this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I also not a big fan of such idea, but let's do for this PR.

@olivierperez olivierperez merged commit 4fd0100 into ChuckerTeam:develop Oct 14, 2019
@olivierperez
Copy link
Member

Thanks, again 👍

@vbuberen vbuberen deleted the update/activity_start branch January 17, 2020 22:01
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