Add links to switch between different reports in left nav-bar #1477

weiwei-lin · 2017-01-16T23:21:12Z

Allow users to navigate between different report. Part of Project Perf-X #1143.
Screenshot of the new left nav-bar:

samuelli · 2017-01-17T00:08:50Z

Best to add screenshots in PR description and to any followup changes.

samuelli · 2017-01-17T00:16:52Z

lighthouse-cli/performance-experiment/server.js

+    this._url = url;
+    this._config = config;
+
+    this._root = fs.mkdtempSync(`${__dirname}/experiment-data`);


Is this current directory? Can we rename it to .lighthouse-performance-data-?

Dot directory might make more sense

If its in the current directory, user needs to be able to identify its from lighthouse

at end for the temp suffix to look a bit nicer

What's the size of this data? Could we store it in memory for now?

It's not in CWD. It's under lighthouse/lighthouse-cli/performance-experiment/.
I tested it on YouTube.com, and the results.json was around 2.3 MB. It's pretty big already and could be bigger for some other websites. So I think we should store it in hard drive.
I addded a "-" to the end of the folder prefix. Thanks for the suggestion.

brendankenny · 2017-01-24T00:23:15Z

I assume this PR will also have to be reworked a bit once #1455 lands?

brendankenny

I'll wait for #1455 before completely getting into this, but looking good so far.

brendankenny · 2017-01-24T00:41:25Z

lighthouse-core/report/templates/report-template.html

@@ -61,6 +61,18 @@
            <h1 class="menu__header-title">Lighthouse</h1>
            <div class="menu__header-version">Version: {{lighthouseVersion}}</div>
          </div>
+          {{#each relatedReports}}


Like the config panel, I don't see us using this outside of an interactive mode like perfX. This one will be slightly more awkward, but possible to pull out into a partial?

There might be some fancy handlebars way of including it, but also could do it brute force, something like

{{#if_eq reportContext "perf-x" }} {{> related-reports}} {{else}} <ul class="menu__nav"> {{#each aggregations}} <li class="menu__nav-item"> <a class="menu__link" href="#{{nameToLink this.name}}"> {{ this.name }} </a> </li> {{/each}} </ul> {{/if_eq}}

(including the menu__nav block in the related-reports partial)

Thx for the advice. I will do this after #1455 is landed.

brendankenny · 2017-01-24T00:43:21Z

lighthouse-core/test/report/report-test.js

@@ -34,7 +34,7 @@ describe('Report', () => {
  it('should format generated Time', () => {
    const reportGenerator = new ReportGenerator();
    const html = reportGenerator.generateHTML(sampleResults);
-    assert.ok(/on 11\/\d{1,2}\/2016\, /gim.test(html));
+    assert.ok(/on: 11\/\d{1,2}\/2016\, /gim.test(html));


hmm, how did this pass before?

I also am curious about this. @WeiweiAtGit do you know?

ah, it was matching on the footer, which has Generated by <b>Lighthouse</b> {{lighthouseVersion}} on {{generatedTime}}

brendankenny · 2017-01-24T00:51:30Z

lighthouse-cli/performance-experiment/server.js

+    this._url = url;
+    this._config = config;
+
+    this._root = fs.mkdtempSync(`${__dirname}/experiment-data-`);


reason for the ending -?

Just to make it looks nicer. Since fs.mkdtempSync will adds some random characters as postfix.

ah, nice, didn't know fs.mkdtempSync existed :)

weiwei-lin · 2017-01-24T01:42:47Z

I assume this PR will also have to be reworked a bit once #1455 lands?

Yes.

paulirish · 2017-01-24T23:55:29Z

Previously, I thought we had discussed using assetSaver.getFilenamePrefix for the id, rather than an incrementing integer. I'd prefer that slightly, as "cool uris don't change"

weiwei-lin · 2017-01-27T00:42:30Z

Ready for review. PTAL

weiwei-lin · 2017-01-27T00:45:12Z

lighthouse-core/report/report-generator.js

-    }
-    return formatter.format(new Date(date));
+    // format time
+    Handlebars.registerHelper('format_time', function(date) {


make this a helper so I can use it in perf-x-left-nav partial

weiwei-lin · 2017-01-27T00:46:22Z

lighthouse-core/report/report-generator.js

@@ -323,7 +330,7 @@ class ReportGenerator {
    return template({
      url: results.url,
      lighthouseVersion: results.lighthouseVersion,
-      generatedTime: this._formatTime(results.generatedTime),
+      generatedTime: results.generatedTime,


pass generated time as time instead of string to ease time comparison in perf-x-left-nav

paulirish · 2017-01-27T23:31:46Z

lighthouse-cli/performance-experiment/experiment-database/database.js

+    this._config = config;
+    this._defaultId = undefined;
+
+    this._root = fs.mkdtempSync(`${__dirname}/experiment-data-`);


rename to _fsRoot ?

fixed. thx for your advice : )

paulirish · 2017-01-27T23:33:55Z

lighthouse-cli/performance-experiment/experiment-database/database.js

+    const perfXReportGenerator = new PerfXReportGenerator();
+
+    const results = JSON.parse(fs.readFileSync(path.join(this._root, id, 'results.json'), 'utf8'));
+    const relatedReports = Object.keys(this._timeStamps)


otherReports as they aren't necessarily related, but they are the only other ones the server knows about, right?

paulirish · 2017-01-27T23:35:28Z

lighthouse-cli/performance-experiment/experiment-database/database.js

+   * @param {?string} id When id is not provided, this._defaultId will be used
+   */
+  getHTML(id) {
+    id = id || this._defaultId;


i dont really follow what the purpose is of the defaultId. Seems like we don't need a fallback?

this allows users to access report from URL bar without inputting a long report id (when they accidentally closed the report tab). Although they can just go to history page most of the time and they still need to remember the port, I feel it's nice to have this feature. But if you feel it's confusing, I can remove it.

Ah. I see. it's a fallback solution so navigating to the web root will always give them a page to use.

Seems okay since we dont have a "please select a report on the left to view" index page yet. cool.

I'd prefer this is handled at the router level rather than the database one. can we move it to the server?

moved to server.js. Thx for the advice.

paulirish · 2017-01-27T23:37:26Z

lighthouse-cli/performance-experiment/report/partials/left-nav.html

+  {{/each}}
+{{/createRelatedReportsContext}}
+<a class="menu__report-tab" href="#">
+ <p class="menu__report-tab__url" title="{{ url }}">Report for: {{ url }}</p>


you should probably run this URL through URL.getDisplayName in the url-shim

I tried this. It didn't fit very well imo. When I tested http://localhost:8080, the url here simply became /. Just to clarify, the URL in Report for: {{ url }}` is the URL of the website being tested, not the report URL.
By the way, originally, the tabs in the left nav were meant to replace the meta data on the top bar. Since we are going to keep the top bar, shall we use report ID here?

paulirish · 2017-01-27T23:41:05Z

lighthouse-cli/performance-experiment/report/partials/left-nav.html

+}
+</style>
+
+{{#createRelatedReportsContext}}


i think we can do this partial better.

one approach: single loop through the reports. check if the reportId matches the requestedID and if it does, tweak the menu__report-tab link. maybe generate the menu__nav's for all the reports, but display:none the others with some classes/css

probably some better ways than that, but let's DRY this up some.

fixed. thx for the advice.

paulirish · 2017-01-27T23:52:48Z

lighthouse-core/report/templates/report-template.html

-            </li>
-            {{/each}}
-          </ul>
+          {{#if_eq reportContext "perf-x" }}


would it be possible to branch this template not on reportContext, but on the data provided to the template?

so we have a partial for this leftnav thing... we have _relatedReports or w/e, then we display all those, otherwise, simple menu__nav-items.

Yes, we can do this. But that also means we'd better move the left-nav partial to the core folder or even embed it into report-template.html directly, because it's no longer just for perf-x in that case (although only perf-x uses it for now). I'm not sure whether I should put the partial in formatters (all the partials there are related to audits), under perf-x, in report-template.html or in a separate folder under core/report. Do you have a good place in mind? And shall we support multiple reports in core/report-generator.js directly in that case?

Based on the loop refactor it seems like the left-nav partial should be pretty straightforward. I like tossing it into the report-template.html directly.

And yeah, report-generator can take the otherReports array now and handle that. Seems good!

instead of otherReports array, I used reportsCatalog. Because the nav needs to know which report is selected currently. Adding a boolean flag to each of the element in the report is another approach, but I feel using reportsCatalog is safer, because we won't accidentally mark multiple reports as selected.

paulirish · 2017-01-27T23:53:03Z

lighthouse-core/test/report/report-test.js

@@ -34,7 +34,7 @@ describe('Report', () => {
  it('should format generated Time', () => {
    const reportGenerator = new ReportGenerator();
    const html = reportGenerator.generateHTML(sampleResults);
-    assert.ok(/on 11\/\d{1,2}\/2016\, /gim.test(html));
+    assert.ok(/on: 11\/\d{1,2}\/2016\, /gim.test(html));


I also am curious about this. @WeiweiAtGit do you know?

paulirish

I think we're in a good shape here. Just some fairly rote changes left and it'll be ready to land!

brendankenny · 2017-01-31T00:42:53Z

lighthouse-cli/performance-experiment/experiment-database/index.js

@@ -0,0 +1,3 @@
+'use strict';
+
+module.exports = require('./database');


any reason to have this file and not just do const ExperimentDatabase = require('./experiment-database/database');?

Because I wanted to encapsulate the database (i.e. database and the temp file it generated will be treated as a whole). Although it will work perfectly as well if we just do const ExperimentDatabase = require('./experiment-database/database');

+1 for not having an empty index.js

paulirish

Nice. I really like the new createNav inline partial and handling there.

A few more ideas in the comments.

Also, should we have some tests? I think database.js is rather testable. And I can see us having a test for the server's reportRequestHandler, providing a fake request/response and asserting that the data provided to response.end includes a few key text strings. EDIT: If you want, we can do these tests in a followup PR.

paulirish · 2017-01-31T00:32:02Z

lighthouse-cli/performance-experiment/server.js

@@ -94,8 +97,19 @@ function requestHandler(request, response) {

 function reportRequestHandler(request, response) {
  try {
+    const id = request.parsedUrl.query.id || defaultReportId;


lets make a little getSelectedId(request) method for this and the other handlers

paulirish · 2017-01-31T00:37:57Z

lighthouse-cli/performance-experiment/server.js

+
+    const reportsMetadata = Object.keys(database.timeStamps).map(key => {
+      const generatedTime = database.timeStamps[key];
+      return {url: database.url, reportHref: `/?id=${key}`, generatedTime};


i think can keep the URL building out of here and only in the template for a cleaner view. Here's a proposal (this and the following comments).

return {url: database.url, reportId: key, generatedTime};

I think it's better to define URL format only in perf-x server. Because if we somehow want to change the URL format in the future, we only need to make changes in perf-x server.
report-template.html does not need to know how routing is done in perf-x server. So there will be less coupling between perf-x server and report template.
Also, other clients will be able to pass reports in a different URL format to the generator if we build URL in the server side.

paulirish · 2017-01-31T00:38:33Z

lighthouse-cli/performance-experiment/server.js

+      const generatedTime = database.timeStamps[key];
+      return {url: database.url, reportHref: `/?id=${key}`, generatedTime};
+    });
+    const reportsCatalog = {reportsMetadata, selectedReportHref: `/?id=${id}`};


const reportsCatalog = {reportsMetadata, selectedReportId: id};

paulirish · 2017-01-31T00:40:17Z

lighthouse-core/report/templates/report-template.html

-            {{> perf-x-reports-nav}}
+          {{#if reportsCatalog }}
+            {{#each reportsCatalog.reportsMetadata }}
+              <a class="menu__report-tab" href="{{ reportHref }}">


... href="/?id={{ reportId }}">

paulirish · 2017-01-31T00:40:43Z

lighthouse-core/report/templates/report-template.html

+                <p class="menu__report-tab__url" title="{{ url }}">Report for: {{ url }}</p>
+                <p class="menu__report-tab__time">Generated on: {{ format_time generatedTime }}</p>
+              </a>
+              {{#if_eq reportHref ../reportsCatalog.selectedReportHref }}


{{#if_eq reportId ../reportsCatalog.selectedReportId }}

paulirish · 2017-01-31T00:45:08Z

lighthouse-cli/performance-experiment/server.js


-
+let database;
+let defaultReportId;


can we call this fallbackReportId ?

changed. thx.

weiwei-lin · 2017-01-31T01:04:10Z

EDIT: If you want, we can do these tests in a followup PR.

Sure : )

paulirish · 2017-01-31T22:39:01Z

lighthouse-cli/performance-experiment/report/scripts/perf-x.js

@@ -31,6 +31,8 @@ class ConfigPanel {
    this._messageField = this._configPanel.querySelector('.js-message');
    this._urlBlockingList = this._configPanel.querySelector('.js-url-blocking-patterns');
    this._urlBlockingStatus = {};
+    const match = location.search.match(/[?&]id=([^&]*)/);
+    this._reportId = match ? match[1]: '';


this._reportId = new URL(window.location).searchParams.get('id');

Didn't know that. Thx for your advice :)

…allback to server.js

paulirish · 2017-01-31T22:55:11Z

lighthouse-cli/performance-experiment/server.js

+      const generatedTime = database.timeStamps[key];
+      return {url: database.url, reportHref: `/?id=${key}`, generatedTime};
+    });
+    reportsMetadata.sort((metadata1, metadata2) => {


something like this should work:

reportsMetadata.sort((a, b) => b.generatedTime - a.generatedTime);

(maybe flip the two if the order is wrong ;)

weiwei-lin force-pushed the left-panel branch from dc43a23 to 3127aab Compare January 16, 2017 23:55

samuelli reviewed Jan 17, 2017

View reviewed changes

weiwei-lin force-pushed the left-panel branch from a4cb057 to 4c60003 Compare January 17, 2017 05:26

samuelli approved these changes Jan 17, 2017

View reviewed changes

samuelli requested a review from paulirish January 17, 2017 05:33

ebidel added the report label Jan 17, 2017

brendankenny requested changes Jan 24, 2017

View reviewed changes

weiwei-lin mentioned this pull request Jan 24, 2017

URL blocking patterns configuration UI #1455

Merged

weiwei-lin force-pushed the left-panel branch 4 times, most recently from 7695d47 to 54c5e2e Compare January 27, 2017 00:40

weiwei-lin commented Jan 27, 2017

View reviewed changes

paulirish requested changes Jan 27, 2017

View reviewed changes

paulirish reviewed Jan 30, 2017

View reviewed changes

brendankenny reviewed Jan 31, 2017

View reviewed changes

paulirish requested changes Jan 31, 2017

View reviewed changes

left-panel Prototype

5823c3b

paulirish reviewed Jan 31, 2017

View reviewed changes

weiwei-lin added 5 commits February 1, 2017 09:39

improved UI

acc8249

fixed a test case

addde15

fixed some coding style

68522ea

fixed some coding style

deaa671

fixed some bugs after rebase

6a77462

weiwei-lin and others added 6 commits February 1, 2017 09:40

fix some coding style and add some comments

265e08e

moved perf-x left nav to partial

9388add

fixed some coding style

3e83ec1

moved left-nav back to core/report-template.html and move report ID f…

f424d5f

…allback to server.js

removed some uncessary code and sort report catalog

372eeb9

fix some coding style

efef4a2

weiwei-lin force-pushed the left-panel branch from dd18955 to efef4a2 Compare January 31, 2017 22:47

paulirish reviewed Jan 31, 2017

View reviewed changes

fixed some coding style and time not being formated after rebase

260ca21

weiwei-lin force-pushed the left-panel branch from df9587b to 260ca21 Compare January 31, 2017 23:05

paulirish approved these changes Jan 31, 2017

View reviewed changes

paulirish merged commit 527bf8e into GoogleChrome:master Jan 31, 2017

		@@ -0,0 +1,3 @@
		'use strict';

		module.exports = require('./database');



		let database;
		let defaultReportId;

Add links to switch between different reports in left nav-bar #1477

Add links to switch between different reports in left nav-bar #1477

Conversation

weiwei-lin commented Jan 16, 2017 • edited Loading

samuelli commented Jan 17, 2017

Choose a reason for hiding this comment

weiwei-lin Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 24, 2017

brendankenny 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

weiwei-lin commented Jan 24, 2017

paulirish commented Jan 24, 2017

weiwei-lin commented Jan 27, 2017

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

paulirish Jan 30, 2017 • edited Loading

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

paulirish 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

paulirish left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwei-lin Jan 31, 2017 • edited Loading

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

weiwei-lin Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

weiwei-lin commented Jan 31, 2017

paulirish Jan 31, 2017 • edited Loading

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

weiwei-lin commented Jan 16, 2017 •

edited

Loading

weiwei-lin Jan 17, 2017 •

edited

Loading

paulirish Jan 30, 2017 •

edited

Loading

paulirish left a comment •

edited

Loading

weiwei-lin Jan 31, 2017 •

edited

Loading

weiwei-lin Jan 31, 2017 •

edited

Loading

paulirish Jan 31, 2017 •

edited

Loading