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

Network track for the timeline #1189

Merged
merged 6 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@gregtatum
Member

gregtatum commented Aug 9, 2018

Deploy preview

I'm pushing this up at the end of the day, feel free to look at the deploy preview, but I plan on reading through it all again with some fresh eyes tomorrow before asking for review.

Edit: This is now ready for review, @ola, would you feel comfortable reviewing this? I have split the work into multiple logical commits, so it will be easiest to look at each in turn, rather than the whole at once. I still haven't reproduced the first-paint effect Markus describes below.

Also resolves #1196

@mstange

This comment has been minimized.

Collaborator

mstange commented Aug 9, 2018

There seems to be an odd first-paint effect where the whole canvas is filled with requests:
screen shot 2018-08-09 at 6 47 24 pm
screen shot 2018-08-09 at 6 47 33 pm

@mstange

This comment has been minimized.

Collaborator

mstange commented Aug 9, 2018

Wheel scrolling doesn't seem to work while the mouse is over the network track.

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 10, 2018

There seems to be an odd first-paint effect where the whole canvas is filled with requests

@mstange: I can't reproduce this. I tried recording my screen, and then loading both from the add-on, viewing existing profiles, and committing new ranges.

@gregtatum gregtatum force-pushed the gregtatum:network-track branch from 9cca8f3 to 8a1aa4a Aug 10, 2018

@codecov

This comment has been minimized.

codecov bot commented Aug 10, 2018

Codecov Report

Merging #1189 into master will increase coverage by 0.15%.
The diff coverage is 85.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1189      +/-   ##
==========================================
+ Coverage    75.9%   76.06%   +0.15%     
==========================================
  Files         144      145       +1     
  Lines        9364     9501     +137     
  Branches     2319     2350      +31     
==========================================
+ Hits         7108     7227     +119     
- Misses       2013     2031      +18     
  Partials      243      243
Impacted Files Coverage Δ
src/types/actions.js 0% <ø> (ø) ⬆️
src/test/fixtures/mocks/canvas-context.js 94.73% <ø> (ø) ⬆️
src/reducers/app.js 96.42% <100%> (+0.59%) ⬆️
src/components/timeline/GlobalTrack.js 80% <100%> (+0.28%) ⬆️
src/reducers/url-state.js 96.74% <100%> (+0.02%) ⬆️
src/profile-logic/tracks.js 94.08% <100%> (+1.01%) ⬆️
src/test/fixtures/profiles/make-profile.js 96.64% <100%> (+0.11%) ⬆️
src/actions/profile-view.js 89.87% <71.05%> (-3.43%) ⬇️
src/components/timeline/TrackNetwork.js 83.01% <83.01%> (ø)
src/reducers/profile-view.js 94.7% <88.23%> (-0.26%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b84472...fc86aa1. Read the comment docs.

@gregtatum gregtatum force-pushed the gregtatum:network-track branch from 8a1aa4a to 4d790ad Aug 10, 2018

@gregtatum gregtatum requested a review from zoepage Aug 10, 2018

expect(getRightClickedTrack(getState())).not.toEqual(trackReference);
expect(getSelectedThreadIndex(getState())).not.toBe(threadIndex);
expect(getLocalTrackRow().hasClass('selected')).toBe(false);
describe('with a thread track', function() {

This comment has been minimized.

@gregtatum

gregtatum Aug 10, 2018

Member

I ended up taking Nicolas Chevobbe's review advice here and re-organized these test files, as I was feeling lost while working on the tests. The setup functions are located at the bottom of the page.

I would recommend turning on "hide whitespace changes" in the Diff Settings on this review, as there is an indentation change.

view.update();
expect(view.find('.timelineTrackHidden').exists()).toBe(true);
expect(view.find('.timelineTrack').exists()).toBe(false);
});
});

This comment has been minimized.

@gregtatum

gregtatum Aug 10, 2018

Member

The following are the new tests:

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 14, 2018

Wheel scrolling doesn't seem to work while the mouse is over the network track.

This is fixed in the latest code.

@zoepage

This PR looks really good. :)

Besides the one question above, I realized that the markers in the network panel get updated when selecting a range, but the list is not updated (see screenshot, checked with following profile)

screen shot 2018-08-17 at 12 23 28

When you select a range for the marker-table, the markers as well as the list get updated.

========

When selecting just the network thread, the shadow makes it a bit hard so see the network markers in the thread. It also feels dense.

screen shot 2018-08-17 at 12 18 40

========

How would you feel about adding one more row or some additional padding at the bottom of the network thread (so increase the min-height), as the thread seems to get "swallowed" when it does not contain markers?

screen shot 2018-08-17 at 12 18 19

type: 'Network',
id,
pri: 0,
status: 'DONE',

This comment has been minimized.

@zoepage

zoepage Aug 17, 2018

Collaborator

The status DONE got me confused as I thought we just have START and STOP, so I thought I rather check in if there is something I am missing? :)

This comment has been minimized.

@gregtatum

gregtatum Aug 20, 2018

Member

It was just mock data, I didn't remember what the strings were off the top of my head, since they aren't in our types. I'll change it to STOP to be consistent and not confusing.

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 20, 2018

When selecting just the network thread, the shadow makes it a bit hard so see the network markers in the thread. It also feels dense.

The shadow is created by the OverflowEdgeIndicator component. It should only show up when the timeline can scroll down, which this is not doing in your screenshots. I narrowed it down to an STR. I think we should fix this in a follow-up, I filed that as #1205.

Besides the one question above, I realized that the markers in the network panel get updated when selecting a range, but the list is not updated (see screenshot, checked with following profile)

This is #865.

How would you feel about adding one more row or some additional padding at the bottom of the network thread (so increase the min-height), as the thread seems to get "swallowed" when it does not contain markers?

Yeah, I can throw another row in there for that case. I feel like the network track being empty is a bit weird, although I think having it disappear might be confusing. Perhaps having it hide itself is a better option here?

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 20, 2018

@zoepage I added 11e9b7e to address your review comments, and filed the follow-up #1205 to deal with the shadow issue. This is ready for your re-review now.

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 20, 2018

I have a PR #1209 for addressing the shadow issue #1205.

@zoepage

This comment has been minimized.

Collaborator

zoepage commented Aug 28, 2018

Yeah, I can throw another row in there for that case. I feel like the network track being empty is a bit weird, although I think having it disappear might be confusing. Perhaps having it hide itself is a better option here?

+1 in hiding itself when it is empty. Do you want to file a follow up for this?

In case we have just very few markers and the row is still min-height, we can re-check again when it landed and file a follow up bug, if this still feels like it gets "swallowed".

Thank you for the changes :)

@gregtatum gregtatum force-pushed the gregtatum:network-track branch from 11e9b7e to fc86aa1 Aug 29, 2018

@gregtatum gregtatum merged commit 6e5306b into devtools-html:master Aug 29, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 85.09% of diff hit (target 75.9%)
Details
codecov/project 76.06% (+0.15%) compared to 0b84472
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment