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

report(redesign): design review feedback #8785

Merged
merged 23 commits into from
May 6, 2019
Merged

report(redesign): design review feedback #8785

merged 23 commits into from
May 6, 2019

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented May 2, 2019

#8185

See feedback under "Round two": https://docs.google.com/document/d/15L5iaWo4ieouegpSlaqGGWpA13JnTv73xnpxuVhzkBg/edit

See commit descriptions for whats up in this PR

anchorElement.addEventListener('click', e => {
e.preventDefault();
window.history.pushState({}, '', anchorElement.hash);
this._dom.find(anchorElement.hash, this._document).scrollIntoView({behavior: 'smooth'});
Copy link
Member

Choose a reason for hiding this comment

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

wait why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was mentioned in the design meeting. @yuinchien asked if the animation could be shorter. i said what if it were just removed completely. i thought everyone said ok

idea was just - user wants to see a particular part of the report, so just show them as soon as intent is made w/o the fanfare of a scroll animation.

Copy link
Member

Choose a reason for hiding this comment

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

yuin and i are still attached to it. i got a branch for fixing the highlighter anim

Copy link
Member

Choose a reason for hiding this comment

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

@paulirish whats up with this

}

.lh-table thead {
background-color: var(--lh-table-higlight-bg);
}
.lh-table thead th {
Copy link
Member

Choose a reason for hiding this comment

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

we had something in there about text alignment with headers

image

  1. getting description flush left with the title.
  2. getting URL flush left with those two.

right, @yuinchien?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm i aligned it to the description instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hoten the title, description, the table heading(URL), the table should all align left to the same line.

Screen Shot 2019-05-03 at 1 47 23 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hoten can we horizontally center the shape (triangle) with the first line of title? Thanks!
Screen Shot 2019-05-03 at 1 48 48 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@connorjclark
Copy link
Collaborator Author

todo
image

@paulirish
Copy link
Member

oops. one more piece on the alignment it seems:

image

the left padding on the th:first-child should be 0 to get "URL" flush with the rest.
and with right paddign on th:last-child

@paulirish paulirish added the P0 label May 5, 2019
@connorjclark
Copy link
Collaborator Author

the left padding on the th:first-child should be 0 to get "URL" flush with the rest.
done

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

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.

4 participants