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

Fix issue #12 (Bug: default value hidden inside fragment) #18

Closed
wants to merge 5 commits into from

Conversation

nasrabadiAM
Copy link

@nasrabadiAM nasrabadiAM commented Jul 3, 2018

I Had same problem that mentioned in this issue, and i used @VrasidasP solution and problem solved.

I replaced this line:
newHeight = measuredHeight
with this:
newHeight = if (measuredHeight == 0) newHeight else measuredHeight.

@DanielMartinus
Copy link
Owner

Thanks! Letting you know I've seen your PR and I will soon test and look at it

RTL problem fixed in this commit, so when you click on minus in rtl mode, counter decrease. and work as expected.
@DanielMartinus
Copy link
Owner

DanielMartinus commented Aug 9, 2018

Updating you that I've been prioritising some other things, sorry for that. Doing a quick scan I'm happy with most changes I see but I'll have to take a bit more time to review though.

Also thanks for addressing the RTL problem

@nasrabadiAM
Copy link
Author

Thank you for announcing me about this,
actually I developed most of these functionalities for my own use, but I will be happy if you review them as quick as you can and give me some feedback about them.

@DanielMartinus
Copy link
Owner

Hi @nasrabadiAM,

It's been a while and before I take on more issues I'm going through the Pull requests first. I want to thank you for the effort and the willingness to contribute.

I have some feedback on the pull request and hope you're willing to address them, if not I can maybe split this pull request. Right now too many things happen in one pull request, I'd prefer to address one issue per pull request. This is because some of the things can be merged and some things need some work/discussion.

  1. Could you open a new branch from master that implements the single commit
    49d1698 that's addressing issue Bug: default value hidden inside fragment #12? That one I'd like to get merged.

  2. Commits b777e15 and 531a451 aren't necessary anymore, this one have been addressed.

  3. Could you open a separate PR with commit b278cab but without all the code formatting and library upgrades? We can then discuss that one separately.

  4. And last, about commit d070872, I rather think this should not be part of this library but the app implementing this widget. How and when the instance state is restored is up to the app implementing this widget.

I currently give the feedback this way so you can make the adjustments and we can continue the discussions in separate threads afterwards but also so you will be credited for your work when things get merged.

@DanielMartinus
Copy link
Owner

DanielMartinus commented Jan 12, 2019

Coming back at my comment:

Could you open a new branch from master that implements the single commit
49d1698 that's addressing issue #12? That one I'd like to get merged.

newHeight = if (measuredHeight == 0) newHeight else measuredHeight.

Is unfortunately not the correct way of fixing this. It'll assign a wrong initial height to the counter. I found a solution to this problem and I'm currently implementing this.

For this reason I'm closing this pull request.

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.

None yet

2 participants