extract tab navigator from app navigator before adding community navigation
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- @albert/tab-navigator
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
You should always put a reviewer on your diffs!! Feel free to ask me next time for pointers on who to pick
| native/navigation/tab-navigator.react.js | ||
|---|---|---|
| 1–129 | Has any of this code changed or is it just moved? | |
Comment Actions
Looking through the git history can be helpful to determine who good reviewers for a diff might be. You can obviously do this with git blame, but Phabricator/Arcanist also provides an arc cover utility that's a little more convenient to use imo.
EG the output of arc cover for this diff:
atul@atuls-MacBook-Pro comm % arc cover Ashoat Tevosyan native/calendar/calendar.react.js: 2 line(s) 56, 67 native/calendar/entry.react.js: 3 line(s) 66, 72-73 native/chat/chat-list.react.js: 1 line(s) 21 native/chat/chat-thread-list.react.js: 2 line(s) 39, 46 native/chat/settings/thread-settings.react.js: 3 line(s) 44, 54-55 native/navigation/app-navigator.react.js: 71 line(s) 3, 9, 11-12, 28, 36, 38, 49, 51, 54-55, 58-60, 62-64, 66-67, 69-71, 74-75, 77-78, 81-82, 86-95, 104-106, 111-115, 119-126, 128-133, 137, 143-147, 246, 248-251 ashoat native/calendar/calendar.react.js: 1 line(s) 68 Tomasz Palys native/chat/chat-list.react.js: 1 line(s) 22 native/chat/chat-thread-list.react.js: 1 line(s) 45 native/navigation/app-navigator.react.js: 1 line(s) 13 Atul Madhugiri native/navigation/app-navigator.react.js: 33 line(s) 4, 10, 61, 65, 68, 76, 79-80, 83-85, 96-103, 107-110, 116-118, 127, 138-142, 247 Jacek Nitychoruk native/navigation/app-navigator.react.js: 1 line(s) 18 KatPo native/navigation/app-navigator.react.js: 7 line(s) 24, 37, 72-73, 134-136
| native/navigation/tab-navigator.react.js | ||
|---|---|---|
| 1–129 | It's mostly moved with a few exceptions, added an export, props to the TabNavigator function and an es-lint disable for the props. The props are because checking the code with flow was yielding errors, though they aren't being used in the file. Cannot create App.Screen element because property navigation is missing and Cannot create App.Screen element because property route is missing | |
| native/navigation/tab-navigator.react.js | ||
|---|---|---|
| 80 | Why do we need to introduce a new prop? Originally, the TabNavigator didn't have any props. If it is really needed, we should do this in a separated diff. When in a diff we move and update the code, it is really hard to review it. | |
| native/navigation/tab-navigator.react.js | ||
|---|---|---|
| 80 | I can see that you mentioned that the purpose of the props is to avoid a flow issue. We need to investigate why the issue occurred and fix the cause of it. | |
| native/chat/settings/thread-settings.react.js | ||
|---|---|---|
| 54 | @tomek question about moving vs updating code in this context. Since this is the result of the new file, it would be allowed. The code changes to watch out for would be more functional (e.g. adding params, side effects, etc.) I don't have access to the Notion yet but Ashoat shared a screenshot earlier. | |
| native/navigation/tab-navigator.react.js | ||
| 80 | Makes sense! I'll dig into it and will try to see who would be good to ping if it's not jumping out. | |
Comment Actions
Pinging this request:
| native/navigation/tab-navigator.react.js | ||
|---|---|---|
| 1–129 | If anything in this huge blob of green has been changed, that should be in a separate diff. It's impossible to review the changes here because it all appears as green | |
Comment Actions
Resigning, feel free to add me back if there's something specific I can be helpful with.