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

feat(webapp): Annotations flot plugin #1605

Merged
merged 29 commits into from Oct 27, 2022
Merged

feat(webapp): Annotations flot plugin #1605

merged 29 commits into from Oct 27, 2022

Conversation

pavelpashkovsky
Copy link
Contributor

@pavelpashkovsky pavelpashkovsky commented Oct 6, 2022

Brief

  • add annotations plugin for timelines

Changes

  • demo screencast:
screencast.2022-10-12.14-33-30.mp4

Concerns

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 445.79 KB (+0.19% 🔺) 9 s (+0.19% 🔺) 4 s (+1.9% 🔺) 12.9 s
webapp/public/assets/app.css 17.95 KB (+0.37% 🔺) 359 ms (+0.37% 🔺) 0 ms (+100% 🔺) 359 ms
webapp/public/assets/styles.css 9.52 KB (+0.17% 🔺) 191 ms (+0.17% 🔺) 0 ms (+100% 🔺) 191 ms
packages/pyroscope-flamegraph/dist/index.js 92.19 KB (0%) 1.9 s (0%) 1.1 s (-20.49% 🔽) 3 s
packages/pyroscope-flamegraph/dist/index.node.js 92.08 KB (0%) 1.9 s (0%) 590 ms (+9.78% 🔺) 2.5 s
packages/pyroscope-flamegraph/dist/index.css 7.4 KB (0%) 148 ms (0%) 0 ms (+100% 🔺) 148 ms

@eh-am
Copy link
Collaborator

eh-am commented Oct 6, 2022

/create-server

@eh-am eh-am added the frontend Mostly JS code label Oct 6, 2022
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 66.59% // Head: 66.61% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (528e96d) compared to base (1332ca5).
Patch has no changes to coverable lines.

❗ Current head 528e96d differs from pull request most recent head 3df0a6b. Consider uploading reports for the commit 3df0a6b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1605      +/-   ##
==========================================
+ Coverage   66.59%   66.61%   +0.03%     
==========================================
  Files         148      146       -2     
  Lines        5030     4965      -65     
  Branches     1162     1142      -20     
==========================================
- Hits         3349     3307      -42     
+ Misses       1676     1653      -23     
  Partials        5        5              
Impacted Files Coverage Δ
...pp/javascript/components/TimelineChart/markings.ts 95.46% <ø> (+2.60%) ⬆️
webapp/javascript/util/formatDate.ts 87.33% <0.00%> (-4.22%) ⬇️
webapp/javascript/redux/reducers/continuous.ts 28.70% <0.00%> (+1.47%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pavelpashkovsky
Copy link
Contributor Author

/create-server

1 similar comment
@Rperry2174
Copy link
Contributor

/create-server

@Rperry2174
Copy link
Contributor

Looks great so far. Let's have the annoatation be a little bit smaller (we can focus on UI / design of it at the end if you'd like)
mockup_annotations_00

@pavelpashkovsky
Copy link
Contributor Author

/create-server

@Rperry2174
Copy link
Contributor

/create-server

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

Curious to hear your thoughts on why you preferred to draw on the canvas VS rendering Icons using React.


I think shouldStartAnnotationsFunctionality is unnecessary. If it's empty we don't render annotations, if it's not empty we render them. An array either has items or does not has items. Adding a third null state is unnecessary. If we need to control whether the entire plugin is activated or not, then we can create another variable enableAnnotationsPlugin or something, although I don't think this is necessary.


I know you have your own preferences, but for consistency let's use function instead of lambda functions.

webapp/javascript/components/TimelineChart/extractRange.ts Outdated Show resolved Hide resolved
webapp/javascript/pages/ContinuousSingleView.tsx Outdated Show resolved Hide resolved
@pavelpashkovsky
Copy link
Contributor Author

@eh-am annotation marks (square + icon) moved outside of Flot, now it's React Component. I found a way how to position them over the timeline correctly. it used to be a problem they lost correct positioning when user changes browser window width

@eh-am
Copy link
Collaborator

eh-am commented Oct 11, 2022

/create-server

@Rperry2174
Copy link
Contributor

Yeah I agree with @eh-am that the "read" modal fields should both have dark backgrounds

https://user-images.githubusercontent.com/6951209/195377738-74f88832-a8ca-4b98-b4c7-a95de64c04e2.png

Maybe eventually we'll add an edit button but that will be separate

@pavelpashkovsky
Copy link
Contributor Author

@eh-am , @Rperry2174 , made this input dark
image

@pavelpashkovsky
Copy link
Contributor Author

/create-server

@eh-am
Copy link
Collaborator

eh-am commented Oct 13, 2022

RE: Icon covering the title:
image

Can you increase the margin a little bit? 20px would work I think
image

@eh-am
Copy link
Collaborator

eh-am commented Oct 13, 2022

Also is it possible to have visual feedback when hovering over an icon? In cases like the following it's difficult to tell which one I am hovering on.
image

Comment on lines +39 to +43
<ContextMenu
click={{ ...pos }}
containerEl={containerEl}
timestamp={Math.round(pos.x / 1000)}
/>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's very interesting, I thought we needed the Provider there since this is rendered outside our normal react app! But it works without it... weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need provider if you want to import any kind of redux logic inside of the body of context menu (for example, redux selectors, useDispatch etc.). but in fact it executes functions which are already were initialized in components connected to redux. You can just take a look at webapp/javascript/pages/ContinuousSingleView.tsx. function dispatch was initialized in ContinuousSingleView, you just execute it in ContextMenu "env"

@pavelpashkovsky
Copy link
Contributor Author

@eh-am sorry, probably I deleted comments accidentally during dev process and then forgot to revert them

@pavelpashkovsky
Copy link
Contributor Author

/create-server

@pavelpashkovsky pavelpashkovsky marked this pull request as ready for review October 17, 2022 14:45
@pavelpashkovsky pavelpashkovsky changed the title feat(webapp): Annotations flot plugin [WIP] feat(webapp): Annotations flot plugin Oct 17, 2022
@Rperry2174
Copy link
Contributor

/create-server

@pavelpashkovsky
Copy link
Contributor Author

/create-server

Copy link
Contributor

@Rperry2174 Rperry2174 left a comment

Choose a reason for hiding this comment

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

lgtm!

@Rperry2174 Rperry2174 merged commit fe80686 into main Oct 27, 2022
@Rperry2174 Rperry2174 deleted the flot-annotations branch October 27, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Mostly JS code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants