Skip to content

[NavigatorIOS] Fix pop method to properly refresh the navigatorBar - #1989

Closed
jaysalvat wants to merge 1 commit into
react:masterfrom
jaysalvat:patch-1
Closed

jaysalvat wants to merge 1 commit into
react:masterfrom
jaysalvat:patch-1

Conversation

@jaysalvat

Copy link
Copy Markdown
Contributor

When using nativator.pop() the navigatorBar doesn't refresh properly if we change navigationBarHidden.

Current result:

When navigator.pop() is called, the navbar doesn't show up, even avec navigationBarHidden set back to true. See between Statusbar and Movie poster in the example below.

bad

// Not actually updating the indices yet until we get the native
// `onNavigationComplete`.
updatingAllIndicesAtOrBeyond: null,

https://github.com/facebook/react-native/blob/master/Libraries/Components/Navigation/NavigatorIOS.ios.js#L510-L512

Expected result (after fix)

The View need to be refreshed in pop method anyway.

updatingAllIndicesAtOrBeyond: this.state.requestedTopOfStack - n,

Now, navbar appears properly.

good

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 14, 2015
@sahrens

sahrens commented Jul 15, 2015

Copy link
Copy Markdown
Contributor

@ericvicenti, @ide: Any idea if this will cause problems? The previous code with the comment was pretty clear....

@sahrens

sahrens commented Jul 15, 2015

Copy link
Copy Markdown
Contributor

@ide: just mark as accepted when you think this is good to go and I'll import it.

@ide

ide commented Jul 15, 2015

Copy link
Copy Markdown
Contributor

Reading through, this won't cause correctness issues I believe but it may affect performance. Will need to dig in deeper since I'm not very familiar with this code.

@jaysalvat

Copy link
Copy Markdown
Contributor Author

Performance is important, but 'm sure we can't afford loosing navbar on pop().

@fender

fender commented Nov 25, 2015

Copy link
Copy Markdown

+1

I've noticed this bug recently too.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@jaysalvat updated the pull request.

@brentvatne

Copy link
Copy Markdown
Collaborator

@facebook-github-bot shipit

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/972167456166907/int_phab to review.

@ghost ghost closed this in e62471b Jan 3, 2016
FalseLobster pushed a commit to FalseLobster/react-native that referenced this pull request Dec 19, 2023
meta-codesync Bot pushed a commit that referenced this pull request Sep 28, 2026
Summary:
Pull Request resolved: #58720

Internal and OSS Flow disagreed on DOM builtins, so suppressions removed in #1965 were unused in OSS but needed internally.

- .flowconfig: builtin_lib=flowlib-with-lib-dom-d-ts
- Delete copied DOM libdefs flow-typed/environment/{bom,dom,cssom,serviceworkers,streams}.js
- flow-typed/environment/node.js: use globalThis.Blob; drop an unused suppression
- Restore suppressions needed under lib.dom.d.ts Console (index-test.js, resolver-test.js, logToConsole.js) and drop ones now unused
- HMRClient-test.js: mock WebSocket via jest.spyOn(global, 'WebSocket', 'get') instead of writing the readonly global

Changelog: [Internal]

Reviewed By: cortinico

Differential Revision: D122152628
meta-codesync Bot pushed a commit that referenced this pull request Sep 28, 2026
Summary:

Internal and OSS Flow disagreed on DOM builtins, so suppressions removed in #1965 were unused in OSS but needed internally.

- .flowconfig: builtin_lib=flowlib-with-lib-dom-d-ts
- Delete copied DOM libdefs flow-typed/environment/{bom,dom,cssom,serviceworkers,streams}.js
- flow-typed/environment/node.js: use globalThis.Blob; drop an unused suppression
- Restore suppressions needed under lib.dom.d.ts Console (index-test.js, resolver-test.js, logToConsole.js) and drop ones now unused
- HMRClient-test.js: mock WebSocket via jest.spyOn(global, 'WebSocket', 'get') instead of writing the readonly global

Changelog: [Internal]

Reviewed By: cortinico

Differential Revision: D122152628
meta-codesync Bot pushed a commit that referenced this pull request Sep 29, 2026
Summary:

Applies the metro #1989 node.js change (resolveObjectURL returns globalThis.Blob) to the React Native mirror copy, and re-enables flow-defs-sync-test (un-skips the parent diff).

Changelog: [Internal]

Differential Revision: D122321586
meta-codesync Bot pushed a commit that referenced this pull request Sep 29, 2026
Summary:
Pull Request resolved: #58733

Applies the metro #1989 node.js change (resolveObjectURL returns globalThis.Blob) to the React Native mirror copy, and re-enables flow-defs-sync-test (un-skips the parent diff).

Changelog: [Internal]

Differential Revision: D122321586
meta-codesync Bot pushed a commit that referenced this pull request Sep 29, 2026
Summary:
Pull Request resolved: #58733

Applies the metro #1989 node.js change (resolveObjectURL returns globalThis.Blob) to the React Native mirror copy, and re-enables flow-defs-sync-test (un-skips the parent diff).

Changelog: [Internal]

Reviewed By: cortinico

Differential Revision: D122321586

fbshipit-source-id: ddbb9c94e3a67df5545e79c5b583bf1632fe7a94
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants