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

Show all JS files in default view of bundle-size chart #645

Closed
wants to merge 1 commit into from
Closed

Show all JS files in default view of bundle-size chart #645

wants to merge 1 commit into from

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Dec 4, 2019

This PR removes the filter so that all JS files are shown in the default view, and instead, adds a suggestion for how to use the filter field.

@@ -30,7 +30,7 @@
<body>
<h1><a href="https://github.com/ampproject/amphtml">AMPHTML</a> bundle-size history</h1>
<div id="chart"></div>
<label for="filter">Filter</label> <input type="text" id="filter" value="v0.js">
<label for="filter">Filter (for example, "v0.js")</label> <input type="text" id="filter" value="">
Copy link
Member

Choose a reason for hiding this comment

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

This is gonna make the chart unreadable in the default view...

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 don't entirely agree. Right now, most information in the chart is hidden.

With this change: For interactive sessions (opening it in a browser), hovering over the chart will highlight different lines, and for non-interactive sessions (showy), you will still see more useful information, particularly inflections in various lines.

WDYT?

bhL3UqsWDLH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to link this with https://github.com/ampproject/amphtml/blob/master/build-system/tasks/bundle-size/APPROVERS.json and only display those files that are there, to encourage WGs to add their files in there ;)

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 like your idea much more than what I was trying to do in this PR. Will abandon this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants