Skip to content

Now allowing the custom nav bar to take up the whole space on iOS#2306

Merged
yogevbd merged 6 commits into
wix:masterfrom
SudoPlz:master
Dec 20, 2017
Merged

Now allowing the custom nav bar to take up the whole space on iOS#2306
yogevbd merged 6 commits into
wix:masterfrom
SudoPlz:master

Conversation

@SudoPlz
Copy link
Copy Markdown
Contributor

@SudoPlz SudoPlz commented Dec 8, 2017

The custom nav bar on iOS was really just a custom view applied to the title of the navigation bar.
There were problems with this approach, because the navigation bar messes up the frame of the title very frequently, especially when the orientation is changing. That behaviour is accepted when the title is a label, but not when we have a custom component.

With this pull request the custom nav bar component can take up the full width of the navigation bar, no matter what orientation we're at.

Here's how the issue looks like in RL:

and here's how it looks after the Pull Request:

Notice how the pink component (Custom Nav Bar component defined in JSX) now takes up the full width of the nav bar.

@guyca guyca requested a review from gran33 December 11, 2017 12:14
@yogevbd yogevbd merged commit 74a02cc into wix:master Dec 20, 2017
chilinh added a commit to chilinh/react-native-navigation that referenced this pull request Dec 21, 2017
* r_master:
  Now allowing the custom nav bar to take up the whole space on iOS (wix#2306)
  Fix formatting of code block in installation-ios (wix#2356)
  Added optional fixedWidth property to the Android side menu (wix#2380)
  await startApp
  on activity resumed (wix#2370)
thanhzusu pushed a commit to thanhzusu/react-native-navigation that referenced this pull request Dec 21, 2017
* master:
  Now allowing the custom nav bar to take up the whole space on iOS (wix#2306)
  Fix formatting of code block in installation-ios (wix#2356)
  Added optional fixedWidth property to the Android side menu (wix#2380)
  await startApp
  on activity resumed (wix#2370)
  fixed toggle tabs bug (wix#2369)
  Add paddingTop to android ToolBar. (wix#2266)
  [Android] TopTabs font family (wix#2342)
  Ensure correct spelling of commandType (wix#2246)
  Add unregisterForPreviewingWithContext (wix#2239)
  Fix rn 0.50 repeated calls to onMeasure. (wix#2261)
  [Android] Support for transparent statusbar with current screen rendered behind it (wix#2274)
  Fix NPE when activity is relaunched on changing font size of the phone (wix#2319)
  Missed comma (wix#2305)
  Add commandType param to props which indicates if a screen is displayed using push or modal
  Find textual buttons in TitleBar by text instead of content description
  Add a toString implementation to StyleParams Color (wix#2313)
guyca added a commit that referenced this pull request Dec 24, 2017
guyca added a commit that referenced this pull request Dec 24, 2017
… iOS" (#2404)

* Revert "Improve getCurrentlyVisibleScreenId on iOS with drawers. (#2052)"

This reverts commit 2c30a52.

* Revert "introducing navbar shadow on iOS (#2339)"

This reverts commit 8fd496e.

* Revert "This fixes an issue with a custom title-view (which has a title and a subtitle) appearing at the left side of the nav-bar instead of the center during a transition animation: when a screen with this title-view is being pushed or when you pop back to it. (#2384)"

This reverts commit 3f7f6c2.

* Revert "Now allowing the custom nav bar to take up the whole space on iOS (#2306)"

This reverts commit 74a02cc.
krystofcelba pushed a commit to krystofcelba/react-native-navigation that referenced this pull request Jan 4, 2018
…x#2306)

* Now allowing the custom nav bar to take up the whole space on iOS

- Also the custom nav bar reacts well to orientation changes
- Fixes: https://github.com/wix/react-native-navigation/issues/1893

* It now works well on iOS 11 as well

* v1.1.314 [ci skip]

* Added optional fixedWidth property to the Android side menu

    - Fixes: https://github.com/wix/react-native-navigation/issues/510
krystofcelba pushed a commit to krystofcelba/react-native-navigation that referenced this pull request Jan 4, 2018
… iOS" (wix#2404)

* Revert "Improve getCurrentlyVisibleScreenId on iOS with drawers. (wix#2052)"

This reverts commit 2c30a52.

* Revert "introducing navbar shadow on iOS (wix#2339)"

This reverts commit 8fd496e.

* Revert "This fixes an issue with a custom title-view (which has a title and a subtitle) appearing at the left side of the nav-bar instead of the center during a transition animation: when a screen with this title-view is being pushed or when you pop back to it. (wix#2384)"

This reverts commit 3f7f6c2.

* Revert "Now allowing the custom nav bar to take up the whole space on iOS (wix#2306)"

This reverts commit 74a02cc.
@DiederikvandenB
Copy link
Copy Markdown

DiederikvandenB commented Jan 9, 2018

Why was this PR reverted? See 1399c7b. I am facing the reported issue too. @guyca

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Jan 9, 2018

Yeah, we really need that fix in our project too. We had to use our own repo because of that revert.

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Jan 10, 2018

@DiederikvandenB @SudoPlz This pr caused buttons to be unresponsive, the custom view was positioned over the buttons preventing them from handling touch events.

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Jan 10, 2018

@guyca in willing to fix that, can you please share an example repo where that issue happens? I'd be grateful.
Thank you!

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Jan 10, 2018

@SudoPlz From what I remember, it was a screen with a custom nav bar component and right/left buttons.

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Jan 10, 2018

Got it thanks @guyca I'll try to reproduce that, edit the commit and resubmit.

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Feb 1, 2018

@guyca I reopened this one. It seems like a false flag, check #2620 out.

@Nickersoft
Copy link
Copy Markdown

Any update on getting #2620 merged/reviewed? I had been facing this same issue when I used this library multiple months ago and have been watching this issue closely since then. I've been sticking to react-navigation until it gets merged, as my app uses a custom gradient navbar.

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Feb 27, 2018

I'm waiting for someone to review for a month now. They're busy working on v2.
Not even sure who to ping.

@Nickersoft
Copy link
Copy Markdown

@SudoPlz Damn that's aggravating... but understandable I suppose. I'd say @guyca but he's been tagged multiple times with no response, so he's pretty busy I imagine. I'm just surprised this was ever an issue to begin with. I guess for now I'll just your fork directly until some progress is made here, as well as report back with any issues if I come across them.

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Feb 27, 2018

I think @guyca deals with Android issues, and that's an iOS issue. Yeah, I'm using my own fork as well until they bake that back in.

@SudoPlz
Copy link
Copy Markdown
Contributor Author

SudoPlz commented Feb 28, 2018

@Nickersoft looks like #2620 was merged.

@Nickersoft
Copy link
Copy Markdown

@SudoPlz Yep! Just saw that. Exciting stuff 😃

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.

6 participants