Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URL blocking patterns configuration UI #1455

Merged

Conversation

weiwei-lin
Copy link
Contributor

@weiwei-lin weiwei-lin commented Jan 12, 2017

Implement UI for URL blocking patterns configuration. Part of #1143
Design Overview:
Toggles in critical request chain section for users to choose which resources to block.
A fixed bottom bar where all the configurations for the next run are shown.

Copy link
Contributor Author

@weiwei-lin weiwei-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't implement the bottom bar yet and still need to fix some bugs. But would like to have some feedback on the UX design first (about the toggle in the critical resources chain section).

Design Overview:

  • Blocked resources (in the previous run) will be stroke out
  • User can click on toggles to choose which URLs to block
    • Red cross: URL will be actively blocked
    • Black cross: resource will be blocked because it's parent will be blocked

Currently, it's not very obvious that those toggles are to configure request blocking patterns in the next run. Later in this PR, a fixed bottom bar will be added. It will show URL blocking patterns update messages.
Unit test for critical request chain is broken for now. Because the html structure of the critical resources tree is changed. Will fix this after we confirmed the UI design.
The bottom bar is not implemented yet.

Could you give me some advice : ) @paulirish @brendankenny @ebidel @samuelli

</style>

{{#*inline "writeNode"}}
<div class="cnc-node" title="{{ this.node.request.url }}">
<div class="cnc-node js-cnc-node" title="{{ this.node.request.url }}" data-blocked="{{ this.node.request.blocked }}" data-to-block="{{ this.node.request.blocked }}">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocked: blocked in this run
to-block: block in next run

{{#createContextFor ../node.children @key ../treeMarkers ../isLastChild ../startTime ../transferSize }}
{{> writeNode this }}
{{/createContextFor }}
{{/each}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the request tree more like a tree in DOM. So we can have a better representation of requests in DOM and make it easier to show correct toggle icon.

@@ -47,6 +47,10 @@ class CriticalRequestChains extends ComputedArtifact {
return false;
}

if (request.statusCode === 0) {
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

treat blocked request as critical
currently, in a normal run, blocked resources must be critical in the previous run (otherwise it's not possible to block it via the UI). This prevents some resources which start with low priority but end up with high priority become non-critical after being blocked. So user will be able to see and configure them in current UI.
This is a temp solution and will probably be removed after I implemented the bottom bar.

@@ -67,7 +71,8 @@ class CriticalRequestChains extends ComputedArtifact {
startTime: request.startTime,
endTime: request.endTime,
responseReceivedTime: request.responseReceivedTime,
transferSize: request.transferSize
transferSize: request.transferSize,
blocked: request.statusCode === 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if there is a better way to tell whether a request is blocked by dev-tool. I will use JavaScript to find all the blocked requests instead if necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, there's a better way. Here's part of the networkRequest object from devtools: https://github.com/ChromeDevTools/devtools-frontend/blob/master/front_end/sdk/NetworkRequest.js#L404-L423

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the link! Updated!

@@ -30,17 +30,39 @@ window.addEventListener('DOMContentLoaded', _ => {
rerunButton.style.display = 'inline-block';

rerunButton.addEventListener('click', () => {
rerunButton.setAttribute('status', 'running');
rerunLighthouse().then(() => {
rerunButton.setAttribute('data-status', 'running');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of data-* since you're not really storing data. These represent state, so I think its fine to have it without the data-* prefix. If its simply for styling, just use a class instead.

rerunButton.classList.add('...')

rerunButton.setAttribute('status', 'running');
rerunLighthouse().then(() => {
rerunButton.setAttribute('data-status', 'running');
rerunLighthouse({blockedUrlPatterns: getUrlPatternsToBlock()}).then(() => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urlPatternsToBlock() or maybe even blockedUrls().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to getUrlBlockingConfig() Because 'blocked' stands for URLs blocked in the previous run in the HTML.

location.reload();
}).catch(err => {
rurunButton.setAttribute('data-status', 'stable');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stable is a little odd to me, since this button will change, probably fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thanks for the suggestion

blockToggles.forEach(toggle => {
toggle.addEventListener('click', () => {
const requestNode = toggle.parentNode;
if (requestNode.getAttribute('data-to-block') === 'true') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just use a class here. This handler will then be
toggle.parentNode.classList.toggle('block-resource')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. Instead of using data URL, changed to request_block (for URL to block in the next run) and request__blocked (for URL blocked in the previous run). thanks for the suggestion.

});
});
});

function getUrlPatternsToBlock() {
const requestNodes = document.querySelectorAll('.js-cnc-node[data-to-block=true]');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will just become .js-cnc-node.block-resource

@paulirish
Copy link
Member

Can you paste in a screenshot or two of the UI to the PR here?

@weiwei-lin
Copy link
Contributor Author

Can you paste in a screenshot or two of the UI to the PR here?

Sure : )

When in --interactive mode, there will be a checkbox next to each critical resource.
unchecked

Users can click the checkbox to block certain resources. The clicked one will show red x, which means this resource is actively blocked. The child resources of blocked resources will show black x, which means they are blocked because their parent is blocked.
checked

If users choose to block some resources and hit rerun, in the new report page, blocked resources will be stroke out.
blocked

If users want to stop blocking this resource in the next run, they can uncheck the checkbox and hit rerun.
blocked-unchecked

@weiwei-lin
Copy link
Contributor Author

weiwei-lin commented Jan 17, 2017

Added bottom config panel.
Screenshots:
bottom-panel-not-expanded

Expanded:
bottom-panel

The rerun button on the top fixed bar is removed since it's no longer needed.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using the critical request chain report for the blocked UI works really well, but I'm wondering if we can avoid making the critical request chain gatherer and audit part of the performance experiment by embedding logic and UI directly in it.

For instance, considering blocked resources as critical includes them in the CRC print out, but isn't necessarily valid for other LH users taking advantage of the blocked URLs option (e.g. if they want to try blocking some key XHR, all of which are currently treated as non-critical).

One approach that would still use all the work that's already been done in this PR would be to make the URL blocking its own audit, separate from the CRC one. It would actually be simpler than that audit, as it would just need to get artifacts.requestCriticalRequestChains and then append networkRecords.filter(req => req.blockedReason() === 'inspector') to it.

The formatter partial and all that could be from the existing CRC audit and the HTML/CSS changes you made in critical-request-chains.html here.

The main downside would be a duplication of critical request chains in the report and the critical request chains in the perfX UI. It should be fairly easy to suppress the output of the CRC audit, either from running at all or just hidden from display.

Just my initial thoughts! WDYT?

@@ -195,10 +195,10 @@ class ReportGenerator {

/**
* Gets the CSS for the report.
* @return {string}
* @return {Array<string>} an array of CSS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return {!Array<string>}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

*/
getReportCSS() {
return fs.readFileSync(path.join(__dirname, './styles/report.css'), 'utf8');
return [fs.readFileSync(path.join(__dirname, './styles/report.css'), 'utf8')];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the isn't used to bring in more than one stylesheet, but was there a reason to make the change? I think it does make sense to move the perfX-specific styling to a separate file where possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to make it more consistent with getReportJS.

@@ -50,6 +50,8 @@ span, div, p, section, header, h1, h2, li, ul {
--report-width: 1280px;
--report-menu-width: 280px;
--report-header-height: 58px;

--config-panel-header-height: 50px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

css once added is difficult to unwind after the fact, so re: above comment, I think a lot of this would be better living in a separate file explicitly for perfX UI, even if it introduces some redundancies.

We're also going to have to think about maintainability, as there are a few proposals about overhauling lighthouse scoring this quarter which will necessitate some level of report structure changes. No one else is really running the performance experiment yet, so we'll need to come up with a to make sure we don't break it.

(this and viewer make me increasingly want to set up a selenium check of the report in travis)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the config panel HTML and related CSS into a partial. Some position related CSS code is still in the main CSS file (now in .report-body__fixed-body-container {...}) so the CSS in the partial does not need to use the variables declared in the main CSS file.
Thx for your advice.

@@ -175,5 +174,27 @@ <h1 class="menu__header-title">Lighthouse</h1>
<footer class="footer">
Generated by <b>Lighthouse</b> {{lighthouseVersion}} on {{generatedTime}} | <a href="https://github.com/GoogleChrome/Lighthouse/issues" target="_blank">File an issue</a>
</footer>

<div class="config-panel js-config-panel">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is nicely self contained, maybe we can move this into a partial, included conditionally on reportContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks for your advice : )

@brendankenny
Copy link
Member

@ebidel since he's been thinking a lot about report unification and maintainability in the last few weeks

@brendankenny
Copy link
Member

One approach that would still use all the work that's already been done in this PR would be to make the URL blocking its own audit, separate from the CRC one

It also may be that doing this as an audit (and included like a normal audit in the report) may be a bit of a hack and we're being blinded by the existing pipeline :)

It'd be easy to append a CRC + blocked resources audit to any config being run as --interactive but not included it in any aggregation, so it wouldn't be printed in the normal report. The UI you've written here could then be inserted into the report in the same way the configuration bar is.

@weiwei-lin
Copy link
Contributor Author

For instance, considering blocked resources as critical includes them in the CRC print out, but isn't necessarily valid for other LH users taking advantage of the blocked URLs option (e.g. if they want to try blocking some key XHR, all of which are currently treated as non-critical).

That's a good point. I did't think of that.

One approach that would still use all the work that's already been done in this PR would be to make the URL blocking its own audit, separate from the CRC one.

I've considered this before. From my point of view, it does not really make sense to add it into audit, because it has nothing to do with the web page performance evaluation. And as you mentioned, we will end up with two almost identical trees in the report page.
What about adding this tree (maybe a complete network request tree) to config panel? We can increase the size of the config panel and add 2 tabs. One tab for text representation and input (just like what we have in config panel now) and the other one for graphical representation and input (just like the CRC with toggle buttons).

Copy link
Contributor Author

@weiwei-lin weiwei-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved most perf-x code to perf-x directory. Minimized the impact on other parts of lighthouse code base.
Moved configurable critical request chain to config tab, leaving the original critical request chain unchanged.
PTAL
Screenshots:
config-panel-tree-view
config-panel-list-view

}

</style>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse critical request chain with some small changes.
The original critical request chain is almost unchanged.

});

// get and recover blocked URL patterns of current run
fetch('/blocked-url-patterns').then(response => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

temporary solution here. once lighthouse report includes runtime configuration, we probably can get the blockedUrlPatterns from there.

@@ -389,6 +380,15 @@ body {
width: calc(100vw - var(--report-menu-width));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Container for bottom panel.
Config panel need some CSS variables here to calculate it's width. This container removes the dependency between config panel and the main CSS.

* Generates the Lighthouse report HTML.
* @param {!Object} results Lighthouse results.
* @param {!string} reportContext What app is requesting the report (eg. devtools, extension)
* @return {string} HTML of the report page.
Copy link
Contributor Author

@weiwei-lin weiwei-lin Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually not a big change here. Just moving formatters registration to it's own method. So I can inject another formatter by overriding this method.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the slow review! Looking good; I think we can move forward with this quite soon.

I think @ebidel is taking a look and will probably have some contradicting comments just to make this process more painful :)

@@ -0,0 +1,37 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2017

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thx

});
});
});
</script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

line-height: 26px;
white-space: nowrap;
display: block;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you revert these last changes to this file now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

function rerunRequestHandler(request, response) {
let message = '';
request.on('data', data => message += data);

request.on('end', () => {
const additionalFlags = JSON.parse(message);
const flags = Object.assign({}, lhParams.flags, additionalFlags);
const flags = Object.assign(lhParams.flags, additionalFlags);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to modify lhParams?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because report page currently needs to ask perf-x server which URL patterns are blocked. So I need to store the blocked URL patterns somewhere. Since in this PR we only store data for one report (including config, flags and results), I decided to directly store it with the flags (AFAIK there is no side effects). In the future PR (#1477), flags will be stored on hard drive (for each experiment) and re-parsed when we need them.

@@ -30,7 +30,7 @@ const http = require('http');
const parse = require('url').parse;
const opn = require('opn');
const log = require('../../lighthouse-core/lib/log');
const reportGenerator = new (require('./report/perf-x-report-generator'))();
const ReportGenerator = require('./report/perf-x-report-generator');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe call this PerfXReportGenerator just to more clearly call it out as an instance of the subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@@ -0,0 +1,158 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


_registerFormatters(audits) {
super._registerFormatters(audits);
const configPanelTemplate = Handlebars.compile(ConfigPanelFormatter.getFormatter('html'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is loading the partial differently than how it's done in _registerFormatters, would it be better to just load the partial file here instead of making ConfigPanelFormatter a Formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thanks for the advice

/**
* @fileoverview Report script for Project Performance Experiment.
*
* Include functions for supporting interation between report page and Perf-X server.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/interation/interaction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thx

* limitations under the License.
*/

/* global window, document, location, fetch */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this file is only for running in the browser, would /* eslint-env browser */ work instead of declaring each?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

</div>

<script>
window.addEventListener('DOMContentLoaded', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this happen here instead of in perf-x.js for a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because it's closely related to config panel and I consider it as part of the UI. But yeah, I can move this to perf-x.js.

@@ -0,0 +1,37 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thx

@@ -0,0 +1,158 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}

.config-panel.expanded .config-panel__panel-toggle {
background-image: url('data:image/svg+xml;utf8,<svg fill="#000000" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg"><path d="M16.59 8.59L12 13.17 7.41 8.59 6 10l6 6 6-6z"/><path d="M0 0h24v24H0z" fill="none"/></svg>');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FF has issues parsing svg data urls with # in them. Can you use fill="black" for these colors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thx

</div>

<script>
window.addEventListener('DOMContentLoaded', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to put script in a partial formatter. As of now, our partial have just been dumb render targets for data. This one is different in that it adds functionality. The script will also be injected every time the partial is included in the report. Presumably that's one time, but it would be good not to go that route :)

Is it possible to do this in your main perf-x.js script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because it's closely related to config panel and I consider it as part of the UI. But yeah, I can move this to perf-x.js.

new ConfigPanel();
});

class ConfigPanel {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define the class before the DOMContentLoaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thx

bodyToggle.addEventListener('click', this._toggleBody.bind(this));

const rerunButton = this._configPanel.querySelector('.js-rerun-button');
rerunButton.addEventListener('click', this._rerunLighthouse.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

}
});
patternInput.addEventListener('keypress', event => {
(event.keyCode || event.which) === 13 && addButton.click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be fine:

if (event.keyCode === 13) {
  addButton.click();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.


// init tree view buttons
const requestBlockToggles = this._configPanel.querySelectorAll('.js-request-blocking-toggle');
requestBlockToggles.forEach(toggle => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sureNodeList.forEach has landed in stable yet (https://www.chromestatus.com/features/5691608351637504).

[...requestBlockToggles].forEach(... for good measure :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Is there any performance overhead for doing that? Because it seems like that converted nodeList to an array.
I changed it to Array.prototype.forEach.call(.... Not sure if this is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea [...requestBlockToggles] is creating an array from the NodeList. You could also do Array.from(requestBlockToggles). Both are more modern ways to Array.prototype.forEach.call. I would prefer the newer options over the latter. Perf will be negligible, esp since the list won't be that long.

Copy link
Contributor Author

@weiwei-lin weiwei-lin Jan 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the newer ones better because they are shorter? If that's the case, what about [].forEach.call?

fetch('/blocked-url-patterns').then(response => {
return response.text();
}).then(data => {
const blockedUrlPatterns = JSON.parse(data);
Copy link
Contributor

@ebidel ebidel Jan 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not parse the response as just? That would save you this JSON.parse call.

fetch('/blocked-url-patterns').then(response => response.json()).then(data => { ... });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thx for the advice

}).then(data => {
const blockedUrlPatterns = JSON.parse(data);
blockedUrlPatterns.forEach(urlPattern => this.addBlockedUrlPattern(urlPattern));
this.log('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intentionally empty?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This is for resetting an .innerHTML?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It is.

});
}

addBlockedUrlPattern(urlPattern) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add docs for these methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

}).then(data => {
const blockedUrlPatterns = JSON.parse(data);
blockedUrlPatterns.forEach(urlPattern => this.addBlockedUrlPattern(urlPattern));
this.log('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This is for resetting an .innerHTML?

@ebidel
Copy link
Contributor

ebidel commented Jan 24, 2017

Haha. Yea, sorry for the duplicate remarks. @brendankenny and I sit right by each other too... :)

@weiwei-lin
Copy link
Contributor Author

Ready for review again. PTAL : )

// init tabs
const tabs = this._configPanel.querySelector('.js-tabs');
const tabNodes = tabs.querySelectorAll('.js-tab');
Array.prototype.forEach.call(tabNodes, tab => {
Copy link
Contributor

@ebidel ebidel Jan 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rock ES6:
[...tabNodes].forEach(

const tabNodes = tabs.querySelectorAll('.js-tab');
Array.prototype.forEach.call(tabNodes, tab => {
tab.addEventListener('click', () => {
Array.prototype.forEach.call(tabs.querySelectorAll('.is-active'), activeEle => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rock ES6:
[...tabs.querySelectorAll('.is-active')].forEach(

});

// init tree view buttons
const requestBlockToggles = this._configPanel.querySelectorAll('.js-request-blocking-toggle');
requestBlockToggles.forEach(toggle => {
Array.prototype.forEach.call(requestBlockToggles, toggle => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

es6

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small styling nit, but this is looking good

const ReportGenerator = require('../../../lighthouse-core/report/report-generator');
const configPanelPartial = fs.readFileSync(path.join(__dirname, 'partials/config-panel.html'),
'utf8');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer four space indent after line break rather than aligning

Copy link
Contributor Author

@weiwei-lin weiwei-lin Jan 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: fixed. thx for the advice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@brendankenny
Copy link
Member

looks like you'll need to rebase because of #1505 as well, sorry

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like this UI. very nice job. :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☑️
☑️
☑️
LGTM!

@brendankenny brendankenny merged commit 3fa2156 into GoogleChrome:master Jan 25, 2017
@weiwei-lin
Copy link
Contributor Author

Thanks!

@weiwei-lin weiwei-lin deleted the configure-blocking-patterns branch January 29, 2017 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants