[Canvas] Switch Canvas to use React Router #100579

crob611 · 2021-05-25T17:59:16Z

Summary

This pull request switches Canvas to use React Router for its routing needs, and all that comes along with that.

Some key changes worth mentioning

We have some state in the app that needs to be reflected in the URL (isFullscreen, etc), but the source of truth of that has previously been in redux. So it's always been tricky to keep those two places N'Sync.

Now, the url will always be the source of truth and we don't even sync that data to redux, opting instead to pass any kind of information or functionality related to routing around as a Context.

Autoplay was a little strange because internally we had two pieces of state, autoplayEnabled and autoplayInterval. So you would have to set the interval and then specifically have to choose autoplay to be on. This is different than how autoRefresh works, where you would just select the refreshInterval and then it would be on. I changed this so that the two match now. Choosing an interval for autoplay automatically turns it on and the url will be updated to match the selected interval.

The view menu without "Stop/Start Autoplay"

Autoplay menu now has an X to disable autoplay, same as auto refresh

There is also a keyboard shortcut for disable autoplay. This would turn autoplay off and the url would be updated so there was no autoplay in it. I changed this slightly so that the keyboard shortcut internally pauses the autoplay, but nothing will be reflected in the url.

This kills A LOT of our redux middleware and most of it is just replaced by pretty simple hooks at the top level of the workpad component. I think it simplifies things a lot.

Testing

This touches a lot, so testing as much as possible is great. Anything that involves routing is going to be the most likely to be impacted, so I'll call out some of the important things
[ ] Undo/Redo on a workpad with Keyboard Shortcuts, Edit Menu links, and browser forward/back navigation
[ ] Changes to workpads that result in a change to the page number you are on. (adding a page, deleting a page, moving a page, etc)
[ ] Creating empty workpads and creating workpads from templates
[ ] Export

elasticmachine · 2021-05-25T17:59:18Z

Pinging @elastic/kibana-presentation (Team:Presentation)

ryankeairns

Approving the SCSS changes and noting that they're unrelated to the form layout issue. I'm not sure when that was broken, but the button and input are misaligned in that screenshot.

crob611 · 2021-05-25T19:22:28Z

@ryankeairns yes thanks for pointing that out. I used the same layout as the AutorefreshControls, which is broken. I thought it might have just been Amsterdam related but it's broken on v7 theme as well. I'll make an issue to follow up on that.

clintandrewhall

Epic work, @crob611 ... some stuff I've mentioned I might leave to a follow-up, but just a stellar job. Let's get it in so we can iterate and avoid tie-ups in CI.

clintandrewhall · 2021-05-28T02:19:43Z

x-pack/plugins/canvas/public/application.tsx

@@ -178,7 +170,4 @@ export const teardownCanvas = (coreStart: CoreStart, startPlugins: CanvasStartDe

  coreStart.chrome.setBadge(undefined);
  coreStart.chrome.setHelpExtension(undefined);
-
-  destroyHistory();
-  stopRouter();
 };


This whole file makes me happy... and it's the first code file I'm reviewing. 🚀

clintandrewhall · 2021-05-28T02:21:27Z

x-pack/plugins/canvas/public/components/app/index.tsx

+import PropTypes from 'prop-types';
+import { History } from 'history';
+// @ts-expect-error
+import createHashStateHistory from 'history-extra/dist/createHashStateHistory';


Um... we should replace this soon, too.

https://www.npmjs.com/package/history-extra

Yep. I left this alone because we will have to switch from Hash routing because the HashHistory that comes from the history package does not allow pushState, which is what the history-extra package allows. It should be a fairly straightforward switch, or we wait for the next version of history which I believe is going to allow state on the hash.

clintandrewhall · 2021-05-28T02:22:24Z

x-pack/plugins/canvas/public/components/app/index.tsx

+import { shortcutManager } from '../../lib/shortcut_manager';
+import { CanvasRouter } from '../../routes';
+
+class ShortcutManagerContextWrapper extends React.Component {


This should be refactored soon, too.

https://reactjs.org/docs/legacy-context.html

Yep, the ReactShortcuts package we use uses the legacy context, hence the need for it here. That package is also not really maintained, so may want to look into other replacements that are more up to date.

clintandrewhall · 2021-05-28T02:22:36Z

x-pack/plugins/canvas/public/components/canvas_loading/canvas_loading.component.tsx

 import { EuiPanel, EuiLoadingChart, EuiSpacer, EuiText } from '@elastic/eui';

-export const CanvasLoading = ({ msg }) => (
+export const CanvasLoading: FC<{ msg?: string }> = ({ msg = 'Loading...' }) => (


clintandrewhall · 2021-05-28T02:23:05Z

x-pack/plugins/canvas/public/components/export_app/export_app.component.tsx

-          <Link name="loadWorkpad" params={{ id }}>
-            Edit Workpad
-          </Link>
+          <RoutingLink to={`/workpad/${id}`}>Edit Workpad</RoutingLink>


This is on the PDF export page, so I don't think it really needs i18n since it's never actually going to be seen. Not sure why that link is even there really.

clintandrewhall · 2021-05-28T02:36:50Z

x-pack/plugins/canvas/public/components/workpad_loader/index.tsx

+      removeWorkpads={removeWorkpads}
+      onClose={onClose}
+      workpads={workpadsState}
+      formatDate={formatDate}


This is easier as:

const { workpadId, canUserWrite } = fromState; <Component downloadWorkpad={onDownloadWorkpad} workpads={workpadsState} {...{ workpadId, canUserWrite, cloneWorkpad, createWorkpad, findWorkpads, removeWorkpads, onClose, formatDate, }} />

I'd love to see consistency in event handlers, (e.g. downloadWorkpad becomes onDownloadWorkpad, createWorkpad becomes onCreateWorkpad, etc)

clintandrewhall · 2021-05-28T02:37:52Z

x-pack/plugins/canvas/public/routes/workpad/hooks/use_fullscreen_presentation_helper.ts

+  const { setFullscreen } = services.platform;
+
+  useEffect(() => {
+    const body = document.querySelector('body');


Isn't this just document.body with the potential of being undefined?

clintandrewhall · 2021-05-28T02:38:45Z

x-pack/plugins/canvas/public/routes/workpad/hooks/use_page_sync.ts

+  }
+
+  useEffect(() => {
+    if (pageIndex !== workpad.page) {


We should consider removing the param from the url if malformed. (follow-up)

clintandrewhall · 2021-05-28T02:40:30Z

x-pack/plugins/canvas/public/routes/workpad/workpad_route.tsx

+    path={'/workpad/:id'}
+    exact={false}
+    children={(route: WorkpadRouteProps) => {
+      return [


nit: can just return the array.

clintandrewhall · 2021-05-28T02:40:51Z

x-pack/plugins/canvas/public/routes/workpad/workpad_route.tsx

+  />
+);
+
+export const ExportRouteManager: FC = ({ children }) => {


I'd almost like to see this split out into individual files.

crob611 · 2021-05-28T15:37:50Z

@elasticmachine merge upstream

kibanamachine · 2021-05-28T15:37:52Z

merge conflict between base and head

crob611 · 2021-05-28T17:19:33Z

@elasticmachine merge upstream

clintandrewhall · 2021-05-28T17:36:01Z

x-pack/plugins/canvas/public/routes/workpad/hooks/use_page_sync.ts

+
+  const pageNumber = parseInt(params.pageNumber, 10);
+  let pageIndex = workpad.page;
+  if (!isNaN(pageNumber)) {


The comment below was actually for ^this line.

We should consider removing the param from the url if malformed. (follow-up)

crob611 · 2021-06-01T12:38:41Z

@elasticmachine merge upstream

kibanamachine · 2021-06-01T21:27:52Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1100	1090	-10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`canvas`	1.3MB	1.3MB	-22.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`canvas`	534.1KB	534.7KB	+575.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`infra`	261	149	-112
`lens`	67	45	-22
`licensing`	18	15	-3
`lists`	239	236	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`securitySolution`	390	346	-44
`stackAlerts`	101	95	-6
total			-342

History

💚 Build #128323 succeeded 2090aa8
💚 Build #128041 succeeded fd2e79b
💚 Build #128005 succeeded 311d7a8
💔 Build #127996 failed 793af7b
💔 Build #127963 failed d541e0d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

poffdeluxe

Approving to unblock! Tested workpads with embeddables and generated lots of PDFs!

Only issue I found was the "undo after fullscreen" issue

* Switch Canvas to use React Router * Fix typescript errors * Remove @scant/router from package.json * Fix tests * Fix functional test * Fix functional tests * Fix bad merge in package.json * Cleanup from code review comments * Fix double basepath append Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-01T21:39:07Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

clintandrewhall · 2021-06-02T00:14:53Z

Thank you @crob611 and @poffdeluxe !!

…sens/kibana into reporting/new-png-pdf-report-type * 'reporting/new-png-pdf-report-type' of github.com:jloleysens/kibana: (46 commits) [Security Solution] Add Ransomware canary advanced policy option (elastic#101068) [Exploratory view] Core web vitals (elastic#100320) [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034) [Lens] Use a setter function for the dimension panel (elastic#101123) [Index Patterns] Fix return saved index pattern object (elastic#101051) [CI] For PRs, build TS refs before public api docs check (elastic#100791) [Maps] fix line and polygon label regression (elastic#101085) Migrate CCR to new ES JS client. (elastic#100131) [Canvas] Switch Canvas to use React Router (elastic#100579) [Expressions] Use table column ID instead of name when set (elastic#99724) [DOCS] Updates docs landing page (elastic#100749) [DOCS] Corrects typo in step 3 (elastic#101079) [DOCS] Updates runtime example in Discover (elastic#100926) Migrate kibana.autocomplete config to data plugin (elastic#100586) [Uptime] New width/delay definition for waterfall sidebar item tooltip (elastic#100147) [FTR] Use importExport for saved_object/basic archive (elastic#100244) [Fleet] Better input for multi text input in agent policy builder (elastic#101020) [CI] Buildkite support with Baseline pipeline (elastic#100492) [Reporting/Telemetry] Do not send telemetry if we are in screenshot mode (elastic#100388) Create API keys with metadata (elastic#100682) ...

kibanamachine · 2021-06-03T21:42:01Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-06-04T21:52:00Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-06-07T22:01:10Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

* Switch Canvas to use React Router * Fix typescript errors * Remove @scant/router from package.json * Fix tests * Fix functional test * Fix functional tests * Fix bad merge in package.json * Cleanup from code review comments * Fix double basepath append Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # package.json

kibanamachine · 2021-06-08T22:11:10Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

Switch Canvas to use React Router

07e6d25

crob611 requested review from a team as code owners May 25, 2021 17:59

ryankeairns approved these changes May 25, 2021

View reviewed changes

Corey Robertson added 6 commits May 25, 2021 16:07

Fix typescript errors

ceede07

Remove @scant/router from package.json

edb8340

Fix tests

e811e3d

Merge branch 'master' into canvas-react-router

9c61f0a

Fix functional test

65f7f76

Fix functional tests

58dcd85

clintandrewhall approved these changes May 28, 2021

View reviewed changes

merge upstream/master

d541e0d

kibanamachine and others added 2 commits May 28, 2021 13:19

Merge branch 'master' into canvas-react-router

793af7b

Fix bad merge in package.json

311d7a8

clintandrewhall reviewed May 28, 2021

View reviewed changes

Cleanup from code review comments

fd2e79b

kibanamachine and others added 2 commits June 1, 2021 08:38

Merge branch 'master' into canvas-react-router

2090aa8

Fix double basepath append

4417680

poffdeluxe approved these changes Jun 1, 2021

View reviewed changes

crob611 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 1, 2021

crob611 merged commit b62848c into elastic:master Jun 1, 2021

kibanamachine mentioned this pull request Jun 1, 2021

[7.x] [Canvas] Switch Canvas to use React Router (#100579) #101120

Closed

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 3, 2021

crob611 mentioned this pull request Jun 8, 2021

[7.x] [Canvas] Switch Canvas to use React Router (#100579) #101659

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 9, 2021

cqliu1 mentioned this pull request Jul 6, 2021

[Canvas] Fix deprecated React lifecycle methods #51751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas] Switch Canvas to use React Router #100579

[Canvas] Switch Canvas to use React Router #100579

crob611 commented May 25, 2021

elasticmachine commented May 25, 2021

ryankeairns left a comment

crob611 commented May 25, 2021

clintandrewhall left a comment

clintandrewhall May 28, 2021

clintandrewhall May 28, 2021

crob611 May 28, 2021

clintandrewhall May 28, 2021

crob611 May 28, 2021

clintandrewhall May 28, 2021

clintandrewhall May 28, 2021

crob611 May 28, 2021

clintandrewhall May 28, 2021

clintandrewhall May 28, 2021

clintandrewhall May 28, 2021

clintandrewhall May 28, 2021

clintandrewhall May 28, 2021

crob611 commented May 28, 2021

kibanamachine commented May 28, 2021

crob611 commented May 28, 2021

clintandrewhall May 28, 2021

crob611 commented Jun 1, 2021

kibanamachine commented Jun 1, 2021

References to deprecated APIs

poffdeluxe left a comment

kibanamachine commented Jun 1, 2021

clintandrewhall commented Jun 2, 2021

kibanamachine commented Jun 3, 2021

kibanamachine commented Jun 4, 2021

kibanamachine commented Jun 7, 2021

kibanamachine commented Jun 8, 2021

[Canvas] Switch Canvas to use React Router #100579

[Canvas] Switch Canvas to use React Router #100579

Conversation

crob611 commented May 25, 2021

Summary

Testing

elasticmachine commented May 25, 2021

ryankeairns left a comment

Choose a reason for hiding this comment

crob611 commented May 25, 2021

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crob611 commented May 28, 2021

kibanamachine commented May 28, 2021

crob611 commented May 28, 2021

Choose a reason for hiding this comment

crob611 commented Jun 1, 2021

kibanamachine commented Jun 1, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

References to deprecated APIs

History

poffdeluxe left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 1, 2021

💚 Backport successful

clintandrewhall commented Jun 2, 2021

kibanamachine commented Jun 3, 2021

kibanamachine commented Jun 4, 2021

kibanamachine commented Jun 7, 2021

kibanamachine commented Jun 8, 2021