[Shared UX] Migrate router from kibana react to shared ux #138544
Conversation
rshen91
changed the title
| "declarationMap": true, | ||
| "emitDeclarationOnly": true, | ||
| "outDir": "target_types", | ||
| "rootDir": ".", |
There was a problem hiding this comment.
This might help.
| "rootDir": ".", | |
| "rootDir": "src", |
clintandrewhall
force-pushed
the
router-migrate
branch
from
eeee04f
to
094b980
Compare
|
@rshen91 I pushed a set of changes to this branch that should fix your issues:
|
…-ref HEAD~1..HEAD --fix'
rshen91
added
the
release_note:skip
rshen91
added
the
Team:SharedUX
| children?: ((props: RouteChildrenProps<any>) => React.ReactNode) | React.ReactNode; | ||
| path?: string | string[]; | ||
| exact?: boolean; | ||
| sensitive?: boolean; |
There was a problem hiding this comment.
Should we add some explanation about what these props are?
There was a problem hiding this comment.
Yes! Added in a31f7b1 🥳
|
|
||
|
|
||
| ```jsx | ||
| <Route basepath="/url" /> |
There was a problem hiding this comment.
Not sure I understand what this example is. basepath doesn't seem to be in RouteProps?
There was a problem hiding this comment.
changed to path - good catch!
a31f7b1
| <Route basepath="/url" /> | ||
| ``` | ||
|
|
||
| This component removes the need for manual calls to `useExecutionContext` and should be removed. |
There was a problem hiding this comment.
*they should be removed?
| import { Route } from './router'; | ||
|
|
||
| describe('Route', () => { | ||
| test('renders', () => { |
There was a problem hiding this comment.
Can we add a more extensive test suite? Given that the render logic depends on a lot of props, would be nice to test some of those scenarios.
There was a problem hiding this comment.
I've added some of the react dom route props in the test suite - let me know what you think. I think for testing exact, sensitive and strict it might involve an integration test?
| /> | ||
| ); | ||
| } | ||
| if (typeof children === 'function') { |
There was a problem hiding this comment.
Can we put this and previous "if" in one if statement, cause they seem to be doing exactly the same thing?
There was a problem hiding this comment.
Wait I could use some clarification - the first if is checking for a render and then rendering props whereas the if on line 54 is checking for the typeof children to be a function and if it is then is using the function from children to render the props (vs render itself). Let me know what I'm missing thanks!
There was a problem hiding this comment.
So the body of the if-statement is exactly the same in both cases. You can do something like:
if (typeof children === 'function' || render) {
const renderFunc = (typeof children === 'function') ? children : render;
return (
<ReactRouterRoute
{...rest}
render={(props) => (
<>
<MatchPropagator />
{renderFunc(props)}
</>
)}
/>
);
}
}
There was a problem hiding this comment.
Thank you for the help! - 0556d16
…-ref HEAD~1..HEAD --fix'
| expect(example).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| test('render prop renders', () => { |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @rshen91 |
kibanamachine
added
v8.5.0
backport:skip
Summary
Building from discussion in #132629, this PR aims to refactor the kibana router and bring it into shared ux. #131805 refactored the router in kibana react and this PR will migrate it to shared ux.
Next Steps for Future PRs
react-router-domusage throughout kibanakibana_react_routerChecklist
Delete any items that are not applicable to this PR.
For maintainers