Fix: render without routing when using as a React component #838
Conversation
Signed-off-by: Tynan DeBold <[email protected]>
There was a problem hiding this comment.
Thanks for setting this up - glad that we got a chance to catch up on this, it looks much better to use the displayGlobalToolbar to single out the Brix use case now :)
It would be great if we can add a test case under wrapper.test.js to test this new use case for the npm component import 😄
Signed-off-by: Tynan DeBold <[email protected]>
Great call. I added a test checking if the |
| it('does not display the settings modal when displayGlobalToolbar is false', () => { | ||
| const wrapper = setup.mount(Wrapper, mockPropsNoGlobalToolbar); | ||
| const container = wrapper.find('.pipeline-settings-modal'); | ||
| expect(container.length).toBe(0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
In looking at this test, this test doesn't serve any purpose / isn't exactly testing anything as the settings modal won't be displayed even if displayGlobalToolbar is true ( .pipeline-settings-modal is only triggered when the modal is triggered and not on the existence of the globalToolbar).
This could be deleted and ideally, we should set up a test to mock the navigation to one of the routes (such as /experiment-tracking) and ensuring that the app doesn't get routed when displayGlobalToolbar is false. ( the test condition would be something like 'does not perform any routing when displayGlobalToolbar is false').
Signed-off-by: Tynan DeBold <[email protected]>
| it('does not perform any routing when displayGlobalToolbar is false', () => { | ||
| const wrapper = setup.mount( | ||
| <MemoryRouter initialEntries={['/experiment-tracking']}> | ||
| <Wrapper /> | ||
| </MemoryRouter>, | ||
| mockPropsNoGlobalToolbar | ||
| ); | ||
|
|
||
| const container = wrapper.find('.pipeline-ui--experiment-tracking'); |
There was a problem hiding this comment.
Thanks for adding this test - this is definitely structured in a much better way.
I've just had a play around with this and tried testing this using mockProps instead of mockPropsNoGlobalToolbar but unfortunately this still passes, which indicates that there is something missing from this setup.
This could either be that we are testing with the wrong css class, or something to do with the setup - worth to do a bit of digging around to find out and set up the right test.
There was a problem hiding this comment.
I'm struggling on this one. I don't see a good way to test this.
Since we'll only render the FlowChartWrapper when we use mockPropsNoGlobalToolbar, we won't have access to any routing, so actually my use of MemoryRouter doesn't make sense here.
And since we don't have access to that routing or even react-router at this stage, I don't see a way to mock navigation to one of the routes using any of the methods you normally would use when routing is present on the page or in the component.
If you have a better idea, do please share it, because I can't see it.
…x/standlone-component-routing Signed-off-by: Tynan DeBold <[email protected]>
…x/standlone-component-routing Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold [email protected]
Description
The Brix team tried to render the component instance of Viz on a page with a route like this:
/post/:id. That didn't work because of the exact path matching we have here. Nothing was rendered.Development notes
Now we use the
displayGlobalToolbarvariable as a switch case: if we display to global toolbar, then we also want all other app elements (UpdateReminder,ExperimentTracking, etc.). If not, we only render theFlowChartWrappercomponent.QA notes
You need to test locally in a dummy repo. You can use
npm linkto do that, but it's tedious. I can show you, as I've already done with @studioswong.Checklist
RELEASE.mdfile