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.

Sign up for GitHub

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

Jump to bottom

Report: denser styling #2706

Closed

paulirish wants to merge 44 commits into master from chowsewhitespaceedits

Member

paulirish commented Jul 17, 2017

This PR takes over from #2297

Basic before and after:

But let's do it with a few reports, because its all about the details:

original:

http://dense-lh-reports.surge.sh/verge.orig.html
http://dense-lh-reports.surge.sh/omroep.orig.html
http://dense-lh-reports.surge.sh/paulirish.orig.html
http://dense-lh-reports.surge.sh/exampleperf.orig.html
http://dense-lh-reports.surge.sh/polymerperf.orig.html

new look:

http://dense-lh-reports.surge.sh/verge.html
http://dense-lh-reports.surge.sh/omroep.html
http://dense-lh-reports.surge.sh/paulirish.html
http://dense-lh-reports.surge.sh/exampleperf.html
http://dense-lh-reports.surge.sh/polymerperf.html

denser still, for devtools:

http://dense-lh-reports.surge.sh/verge.devtools.html
http://dense-lh-reports.surge.sh/omroep.devtools.html
http://dense-lh-reports.surge.sh/paulirish.devtools.html
http://dense-lh-reports.surge.sh/exampleperf.devtools.html
http://dense-lh-reports.surge.sh/polymerperf.devtools.html

chowse and others added 30 commits

May 17, 2017 18:52


          Updated report styles

323349c


          Removed an extra indent and adjusted trailing padding after category …

868454f

…heading


          Moved DevTools styling behind the .lh-devtools class

b336091


          Review fixes

f1f577e


          Removed unneeded description parameter

07c42df


          Merge branch 'master' of github.com:GoogleChrome/lighthouse into whit…

3256d86

…espace


          Fixed eslint errors

bdec2c9


          Fixed failing tests

9d40d68


          Merge remote-tracking branch 'upstream/master' into whitespace

1041cee


          Fixed failing tests

f0991b7


          Merge branch 'master' of github.com:GoogleChrome/lighthouse into whit…

c725c69

…espace


          Merge branch 'master' into chowse-whitespace

cbe08f4


          fix bugs. lol useful commit message

369b152


          move disclosure triangles back to the right

b2f26d1


          don't put details/summary around auditgroups

b47e425


          tables are open by default.

7fc6100


          devtools font size isn't smaller than 12px

42594c7


          Merge branch 'master' into chowsewhitespaceedits

d7253f0


          Revert "move disclosure triangles back to the right"

b5150df

This reverts commit b2f26d1.


          Revert "don't put details/summary around auditgroups"

018c006

This reverts commit b47e425.


          no special styling for the manual section

316de0f


          some groups can't be collapsed: failed, perf groups, a11y failing groups

2f55604


          restore focus ring

813461b


          collapse left nev at 964 instead of 720px

c255224


          keep details open by default.

2ea7f6c


          sharper focus ring 5px -> 3px

021b929


          restore hard width

bbd5cad


          in case of debugString, make sure we still get a toggle arrow.

6b8b247


          break-word for long urls in debugStrings


          for perf opportunities, show debugText within summary, so it isn't pu…

e650cd1

…shed below large results tables.

paulirish added 3 commits

July 17, 2017 15:56


          Merge branch 'master' into chowsewhitespaceedits

d96f17c


          fix tests for categoryrenderer

81ade4e


          fix closure compilation.

38e2e8e

patrickhulce reviewed

View reviewed changes

lighthouse-core/test/report/v2/renderer/category-renderer-test.js Outdated

-                  const audits = categoryDOM.querySelectorAll('.lh-category > .lh-audit, ' +
-                      '.lh-category > .lh-passed-audits > .lh-audit, ' +
-                      '.lh-audit-group--manual .lh-audit');
+                  const audits = categoryDOM.querySelectorAll('.lh-audit');

Collaborator

patrickhulce Jul 18, 2017

❤️

patrickhulce reviewed

View reviewed changes

Collaborator

patrickhulce left a comment

all seems fine to me, but a couple nit thoughts

the debug string feels really close to the description even in non-devtools, should the description text be a bit smaller too?

disclosure triangles different colors (at this point I'll stop because I know that was on your list so you probably aren't ready for review 😄 )

lighthouse-core/report/v2/templates.html Outdated

@@ @@ -265,14 +287,14 @@ <h1 class="leftnav__header__title">Lighthouse</h1> @@
                   .lh-config__settings-toggle summary::-webkit-details-marker {
                     display: none;
                   }
-                  @media screen and (min-width: 1130px) {
+                  @media screen and (min-width: 960px) {

Collaborator

patrickhulce Jul 18, 2017

this meant to be 964px?

Member Author

paulirish Jul 18, 2017

yeah sorted these out. most are 964. there's a 965 in there too.
thx!

paulirish added 6 commits

July 18, 2017 13:31


          same color for details/summary arrows

fcb6767


          shorter sparklines.

933d9f8


          ensmallen scorecards. size based off body-font-size

c292a45


          tweak 964 breakpoint. fix @print styles.

9124df1


          show site URL & time in print view.

cacf55f


          smaller lh-debug size for devtools. margin on top of lh-debug

77ef9bc

Member Author

paulirish commented Jul 18, 2017 •

edited

Loading

the debug string feels really close to the description even in non-devtools

sg. added 3px margin in all cases. feels good.

should the description text be a bit smaller too?

changed it from 12px to 11px in devtools. (this was chris's original size, too)

disclosure triangles different colors

good call. sorted.

paulirish added 2 commits

July 18, 2017 15:00


          firefox support

501811b


          fix intra-page permalink nav. (no more overlapping header)

5443cf4

brendankenny requested changes

View reviewed changes

lighthouse-core/report/v2/renderer/category-renderer.js Outdated

@@ @@ -489,6 +490,11 @@ class CategoryRenderer { @@
                   element.appendChild(passedElem);
                   return element;
                 }
+                _createPermalinkSpan(element, id) {

Member

brendankenny Jul 18, 2017

jsdocs for these to pass the closure check

lighthouse-core/report/v2/renderer/category-renderer.js Outdated

@@ @@ -210,28 +205,30 @@ class CategoryRenderer { @@
                  * Renders the group container for a group of audits. Individual audit elements can be added
                  * directly to the returned element.
                  * @param {!ReportRenderer.GroupJSON} group
+                 * @param {{expandable: boolean}} opts
                  * @return {!HTMLDetailsElement}

Member

brendankenny Jul 18, 2017

should drop HTMLDetailsElement for Element (which breaks el.open but old-type-inference closure doesn't catch that)

lighthouse-core/report/v2/renderer/category-renderer.js Outdated

+                _renderAuditGroup(group, opts) {
+                  const expandable = opts.expandable;
+                  const auditGroupElem = /** @type {!HTMLDetailsElement} */ (
+                        this._dom.createElement(expandable ? 'details' :'div',
                         'lh-audit-group lh-expandable-details'));

Member

brendankenny Jul 18, 2017

Since this is shared by expandable and non-expandable, lh-expandable-details -> lh-group (or something better)? Is lh-audit-group only used for these?

Member Author

paulirish Jul 19, 2017

sg. stripped expandable details from these.

lighthouse-core/report/v2/renderer/category-renderer.js Outdated

                         'lh-audit-group lh-expandable-details'));
-                  const auditGroupHeader = this._dom.createElement('div',
+                  const auditGroupSummary = this._dom.createChildOf(auditGroupElem, 'summary',
+                        'lh-audit-group__summary lh-expandable-details__summary');

Member

brendankenny Jul 18, 2017

lh-expandable-details__summary

lighthouse-core/report/v2/renderer/category-renderer.js Outdated

-                  const auditGroupHeader = this._dom.createElement('div',
+                  const auditGroupSummary = this._dom.createChildOf(auditGroupElem, 'summary',
+                        'lh-audit-group__summary lh-expandable-details__summary');
+                  const auditGroupHeader = this._dom.createChildOf(auditGroupSummary, 'div',
                         'lh-audit-group__header lh-expandable-details__header');

Member

brendankenny Jul 18, 2017

lh-expandable-details__header

lighthouse-core/report/v2/renderer/category-renderer.js Outdated

-                  });
-                  auditGroupSummary.appendChild(auditGroupHeader);
-                  auditGroupSummary.appendChild(auditGroupArrow);
+                  if (group.description) {

Member

brendankenny Jul 18, 2017

another option is to put these as two templates, one expandable and one unexpandable. _renderAuditGroup could then just query the one it needs and then add a description as below if needed

lighthouse-core/report/v2/renderer/category-renderer.js

@@ @@ -167,6 +163,8 @@ class CategoryRenderer { @@
                   const titleEl = this._dom.createChildOf(summary, 'div', 'lh-perf-hint__title');
                   titleEl.textContent = audit.result.description;
+                  this._dom.createChildOf(summary, 'div', 'lh-toggle-arrow', {title: 'See resources'});

Member

brendankenny Jul 18, 2017

can this be lh-toggle-arrow-unexpandable? Since it doesn't appear to expand?

Member Author

paulirish Jul 19, 2017

This is the item level arrow in Opportunities, which is expandable:

afaik, all non expandable arrows correctly have that class.

Member

brendankenny commented Jul 18, 2017

lookin great


          feedback.

b918642

brendankenny approved these changes

View reviewed changes

Member

brendankenny left a comment

LGTM

paulirish added 2 commits

July 19, 2017 16:49


          Merge branch 'master' into chowsewhitespaceedits

c9403cf


          Merge branch 'master' into chowsewhitespaceedits

4f21b78

paulirish added a commit that referenced this pull request


          Report: improvements w/ new density... (#2706)

97c7170

    category hash nav shouldn't underlap the fixed header
    firefox support
    width@964 breakpoint.
    fix @print styles: show site URL & time
    ensmallen scorecards. size based off body-font-size
    shorter sparklines.
    for perf opportunities, show debugText within summary, so it isn't pushed below large results tables.
    break-word for long urls in debugStrings
    sharper focus ring 5px -> 3px
    open details by default
    some groups can't be collapsed: failed, perf groups, a11y failing
    no special styling for the manual section
    devtools font size isn't smaller than 12px

Member Author

paulirish commented Jul 20, 2017

manually squashed and merged in 97c7170

paulirish closed this

paulirish deleted the chowsewhitespaceedits branch

March 2, 2018 23:33

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

Labels

None yet