Bug 1450022 - Convert entire Details Panel to ReactJS #3621

camd · 2018-06-05T22:45:12Z

Moves everything in the details-panel to ReactJS.

Tested:

selecting a job and viewing the summary, job details, failure summary and all tabs
perf tab shows up when it's a non-failed job with perf data
autoclassify tab shows when &autoclassify is in url
classify and delete classification for a job
classify by manually entering a bug number
click bug pin in Failure Summary and the job is pinned with bug selected, save classification
Pinning and unpinning jobs, also "clear all" in the save drop-down of pinboard
Pinning 2 jobs and retriggering in save drop-down
Pinning the in-progress and cancelling all in save drop-down
All options in the "..." menu of ActionBar
retrigger and cancel in ActionBar
Viewing raw and logviewer
with jobs pinned, clicking open area does not clear detail panel
manually opening and closing the pinboard
close button of pinboard
bugfiler opens (didn't actually file a bug)
custom actions dialog opens

Note:

Resizing the bottom panel is broken in prod, so didn't address fixing that
Also there are some cases where esc doesn't close the bottom window. But it, again, seems to be the same as prod, so not addressing now.

edmorley

I haven't had a chance to test this locally yet - will do so tomorrow.

edmorley · 2018-06-07T00:24:42Z

ui/vendor/resizer.js

@@ -1,48 +0,0 @@
-import angular from 'angular';


With this file removed, the ui/vendor/ entry can be removed from .eslintignore (and so that file removed too).

Oh cool, done.

edmorley · 2018-06-07T00:34:27Z

ui/job-view/PushHeader.jsx

+  thPinboardCountError,
+  thEvents,
+  thPinboardMaxSize
+} from "../js/constants";


single quotes

edmorley · 2018-06-07T00:45:00Z

ui/job-view/details/PinBoard.jsx

+    });
+  }
+
+  isSHAorCommit(str) {


Some of these might be good to move to helpers/?

Great point, yeah. I'll move as many static functions as possible them into helpers.

edmorley · 2018-06-07T00:46:28Z

ui/job-view/details/summary/LogUrls.jsx

@@ -0,0 +1,89 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import logviewerIcon from '../../../img/logviewerIcon.svg';


newline before this line

edmorley · 2018-06-07T00:54:45Z

ui/job-view/details/tabs/TabsPanel.jsx

+              ><span className="fa fa-times" /></span>
+            </span>
+          </TabList>
+          <TabPanel>


What are you thoughts on moving <TabPanel> to be inside *Tab.jsx?
(Not sure what's considered best practice for this kind of pattern)

Interesting idea, I'll give it a shot and see how that works. :)

In trying to do this, it ends up that react-tabs checks that there are a matching number of TabPanel to Tab components. If I move TabPanel to inside the tab, like FailureSummaryTab then it doesn't detect there are a matching number of Tab to TabPanel. If I have FailureSummaryTab extend TabPanel rather than React.Component then it DOES work. However, only for some of the tabs. It appears to break tab navigation.

I haven't looked too deeply, but I fear that extending TabPanel may cause state in our component to potentially interfere with TabPanel. It may also be that it's causing some interference with applying classes to show/hide in some rendering code in TabPanel.

I'm thinking this may not be an avenue worth exploring further at this time. What do you think?

Yeah that sounds fine :-)

Btw I can't see the stylesheet being imported anywhere? (https://github.com/reactjs/react-tabs#styling) Would using that reduce the boilerplate we need in our own stylesheet? (If not, don't worry)

Yeah, I'd tried using one, but had to start overriding a pieces of styling we didn't want, so it started to get messy and I just went with what we had been using. :)

camd · 2018-06-07T16:21:53Z

It had also occurred to me in this PR that we have a lot of imports.. :) Maybe agreeing on some general order of them would help. My thoughts were:

Components
Models
Helpers
Constants
Other

What do you think?

edmorley · 2018-06-07T16:34:27Z

In the future we might want to enable this import order eslint rule:
https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md

It doesn't yet sort within the groups (import-js/eslint-plugin-import#389) though.

For now, perhaps just roughly follow the order in that rule's documentation?

camd · 2018-06-07T17:10:44Z

Cool, ok. I think I've modified it to follow that.

I went ahead and started using the order of

Other
Constants
Helpers
Models
Siblings, etc

With a space where there are different directory depths. I hope that's about right. :)

edmorley · 2018-06-08T13:31:51Z

ui/job-view/details/DetailsPanel.jsx

+      const reftestUrl = jobLogUrls.length ?
+        `${getReftestUrl(jobLogUrls[0].url)}&only_show_unexpected=1` :
+        '';
+      const performanceData = flatten(Object.values(results[3]));


After rebasing on master to resolve the conflict in ui/plugins/controller.js (from #3627), could you convert this and the other flatten() usage to ES6? :-)

Absolutely, yeah. Done.

sarah-clements · 2018-06-08T17:29:42Z

ui/job-view/details/DetailsPanel.jsx

+      jobLogUrls: [],
+      jobDetailLoading: false,
+      jobLogsAllParsed: false,
+      lvUrl: null,


Nit: I think it'd be clearer if you spelled out logViewerUrl and logViewerFullUrl rather than abbreviating with 'lv' especially since it's passed down to multiple components.

Good call, yeah.

sarah-clements · 2018-06-08T17:35:49Z

ui/job-view/details/DetailsPanel.jsx

+    this.closeJob = this.closeJob.bind(this);
+    this.countPinnedJobs = this.countPinnedJobs.bind(this);
+    // give access to this count to components that don't have a common ancestor in React
+    // TODO: remove this once we're fully on ReactJS.


Is this TODO filed in bugzilla or mentioned as part of other React conversion bugs? Wondering how we are keeping track of these.

Good point. This is in Bug 1450042. But added that bug number to the TODO to help keep track. In this case, it'll be pretty obvious once all uses of $injector and $rootScope no longer exist.

sarah-clements · 2018-06-08T18:04:56Z

ui/job-view/details/tabs/TabsPanel.jsx

+      this.setState({ tabIndex: nextIndex < tabCount ? nextIndex : 0 });
+    });
+
+    this.setTabIndex = this.setTabIndex.bind(this);


I'm curious why this method is bound here rather than in the constructor.

This appears (as far as I can tell) to be a quirk of mixing angular with react. In some cases, the function gets bound to a state of the app where you get an error on the console like this:

Warning: Can't call setState on a component that is not yet mounted. This is a no-op, but it might indicate a bug in your application. Instead, assign to `this.state` directly or define a `state = {};` class property with the desired state in the TabsPanel component. warning.js:33

So putting it in componentDidMount hits it at a point where it obviously WAS mounted already. I'm hoping to move these all into the constructors when we no longer need react2angular.

I actually referenced this in one of my TODOs here a while back:

treeherder/ui/job-view/details/tabs/autoclassify/AutoclassifyTab.jsx

Line 87 in 44e25f7

// TODO: Once we're not using ng-react any longer and

Ah, I see. Thanks for explaining :)

camd · 2018-06-10T22:17:21Z

The selenium tests are failing for non-timeout reasons this time. :) I changed a few of the CSS classes and ids, which broke the tests. I'll address that this coming week.

camd · 2018-06-13T00:23:03Z

OK, selenium tests are fixed now.

camd force-pushed the pinboard-to-react branch 3 times, most recently from 961393c to 46530dc Compare June 6, 2018 00:11

camd had a problem deploying to treeherder-prototype June 6, 2018 04:47 Failure

camd temporarily deployed to treeherder-prototype June 6, 2018 05:02 Inactive

camd temporarily deployed to treeherder-prototype June 6, 2018 05:15 Inactive

camd force-pushed the pinboard-to-react branch 2 times, most recently from aa1560b to 38d5c95 Compare June 6, 2018 19:11

camd had a problem deploying to treeherder-prototype June 6, 2018 21:34 Failure

camd force-pushed the pinboard-to-react branch from 531480a to ac24c03 Compare June 6, 2018 21:43

camd temporarily deployed to treeherder-prototype June 6, 2018 21:43 Inactive

camd force-pushed the pinboard-to-react branch from 5b701c0 to 1e8c514 Compare June 6, 2018 23:07

camd requested a review from edmorley June 6, 2018 23:07

edmorley reviewed Jun 7, 2018

View reviewed changes

camd changed the title ~~Bug 1450022 - Convert Pinboard to ReactJS~~ Bug 1450022 - Convert entire Details Panel to ReactJS Jun 7, 2018

camd force-pushed the pinboard-to-react branch 2 times, most recently from 520426e to e60264a Compare June 7, 2018 21:28

edmorley mentioned this pull request Jun 8, 2018

Bug 1467504 - convert _uniq() usages to ES6 #3634

Merged

edmorley reviewed Jun 8, 2018

View reviewed changes

camd force-pushed the pinboard-to-react branch from e60264a to 374cfe5 Compare June 8, 2018 17:16

sarah-clements approved these changes Jun 8, 2018

View reviewed changes

camd force-pushed the pinboard-to-react branch from 997c620 to 41df94a Compare June 13, 2018 00:07

camd force-pushed the pinboard-to-react branch 3 times, most recently from 3f97704 to e032f3e Compare June 13, 2018 21:47

Bug 1450022 - Convert Pinboard to ReactJS

e5d9f76

camd force-pushed the pinboard-to-react branch from e032f3e to e5d9f76 Compare June 13, 2018 22:23

camd merged commit 15721f0 into master Jun 13, 2018

camd deleted the pinboard-to-react branch June 13, 2018 22:40

edmorley mentioned this pull request Jun 14, 2018

Bug 1468658 - Fix & enable more ESLint rules #3665

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1450022 - Convert entire Details Panel to ReactJS #3621

Bug 1450022 - Convert entire Details Panel to ReactJS #3621

camd commented Jun 5, 2018 •

edited

Loading

edmorley left a comment

edmorley Jun 7, 2018

camd Jun 7, 2018

edmorley Jun 7, 2018

edmorley Jun 7, 2018

camd Jun 7, 2018

edmorley Jun 7, 2018

edmorley Jun 7, 2018

camd Jun 7, 2018

camd Jun 7, 2018

edmorley Jun 7, 2018

camd Jun 7, 2018

camd commented Jun 7, 2018

edmorley commented Jun 7, 2018 •

edited

Loading

camd commented Jun 7, 2018

edmorley Jun 8, 2018

camd Jun 8, 2018

sarah-clements Jun 8, 2018

camd Jun 8, 2018

sarah-clements Jun 8, 2018

camd Jun 8, 2018

sarah-clements Jun 8, 2018

camd Jun 8, 2018

sarah-clements Jun 8, 2018

camd commented Jun 10, 2018

camd commented Jun 13, 2018

Bug 1450022 - Convert entire Details Panel to ReactJS #3621

Bug 1450022 - Convert entire Details Panel to ReactJS #3621

Conversation

camd commented Jun 5, 2018 • edited Loading

edmorley 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

camd commented Jun 7, 2018

edmorley commented Jun 7, 2018 • edited Loading

camd commented Jun 7, 2018

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

camd commented Jun 10, 2018

camd commented Jun 13, 2018

camd commented Jun 5, 2018 •

edited

Loading

edmorley commented Jun 7, 2018 •

edited

Loading