Hello Harshal,
I passed the patch through our CI and all the tests passed. The changes do not break previous behavior but because there are no tests on the new feature we could not be sure it was really working. So we did some manual testing and sometimes it doesn't work, like it gets stuck in a place and you need to press the shortcut again in order for it to move.
Codewise I have some suggestions:
- dialogTabNavigator looks a nice candidate for a class with its own file. This way we can test the behavior
- There is no difference between a variable called e
and a variable error
so for sake of clarity I would love to see variable names that we can easily read
- We are also using 2 different types of variable naming camelCase and snake_case, if we could use only camelCase on Javascript it would make the code more uniform
- I noticed that there are some linting issues in the Javascript code
Summing it up I believe that despite the feature not working 100% of the time, looks like the code is almost there but needs some refactoring to make it more readable, instead of comments we could have function calls that more clearly state what we are looking for something like DialogTabNavigator.isLastTabOfChild
Thanks
Joao