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

Ensure header elements are visible on iOS #668

Closed
wants to merge 3 commits into from

Conversation

zibs
Copy link

@zibs zibs commented Oct 18, 2020

Adds ensureHeaderButtonsVisible method in RNScreens.m
References issues #432 and #528.
References and incorporates PR #644 as well

Thanks @WoLewicki!

Adds ensureHeaderButtonsVisible method in RNScreens.m
References issues  software-mansion#432 and software-mansion#528.
@WoLewicki
Copy link
Member

WoLewicki commented Oct 19, 2020

Thanks for submitting the PR! The main problem with this method is that it assumes that 0.1-second time is enough for the buttons to align correctly after it. It may not be true for screens with more content in them or slower phones. You can see #528 (comment). Is it available for you to check if this 0.1-second time is always enough for this to work correctly?

@zibs
Copy link
Author

zibs commented Nov 9, 2020

I can get back to you on this this week. It seems like after testing in the wild that 0.1 wasn't enough time when running CPU heavy transitions.

@zibs
Copy link
Author

zibs commented Dec 14, 2020

So I see here that I've left it set at 0.5, which might even be too much. In production we're running it at 0.2 and don't think we've had any issues since. Want me to increase it to 0.2 here? Or just leave it at 0.5?

Other than that it should be good to go.

@WoLewicki
Copy link
Member

I am not sure if we will want to merge it for now. It is very much a workaround, and with some randomness included, so I would be very hesitant about doing so. I will ping here when it is clear if we want to do it. And for now, I think you can leave it as it is 🎉

@casperstr
Copy link

casperstr commented Apr 12, 2021

What is the status on this? @WoLewicki Can I do anything to help move this forward?

@WoLewicki
Copy link
Member

@casperstr see my comment above. I don't think there is much more to be done about it. If someone wants to use it, he should patch-package it for now. I think we would need a deterministic solution in order to merge such a change.

@casperstr
Copy link

@casperstr see my comment above. I don't think there is much more to be done about it. If someone wants to use it, he should patch-package it for now. I think we would need a deterministic solution in order to merge such a change.

The native stack is currently unusable if header buttons are required. Do we have any path forward to patch this with a deterministic solution?

@WoLewicki
Copy link
Member

I haven't found any other solution unfortunately, but I am open to contributions regarding this.

@abram
Copy link
Contributor

abram commented Apr 30, 2021

Hi @zibs, thanks for your work here. I ran into this issue today and I was grateful to find this PR and discussion. @WoLewicki, I've submitted PR #902 containing a solution I came up with. Hope it's helpful!

@WoLewicki
Copy link
Member

I'll close this since #902 is merged and should resolve the problem. Feel free to comment here if something is wrong.

@WoLewicki WoLewicki closed this May 7, 2021
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.

4 participants