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: fix toolbar on narrow screens #1754

Merged
merged 19 commits into from Nov 30, 2022
Merged

Conversation

dogfrogfog
Copy link
Contributor

Brief

Changes

  • add ability to show 2nd toolbar row
  • add "more" button to open collapsed toolbar items

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 65.84% // Head: 66.38% // Increases project coverage by +0.55% 🎉

Coverage data is based on head (498002f) compared to base (c36aa9c).
Patch coverage: 85.72% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1754      +/-   ##
==========================================
+ Coverage   65.84%   66.38%   +0.55%     
==========================================
  Files         170      170              
  Lines        5561     5550      -11     
  Branches     1258     1255       -3     
==========================================
+ Hits         3661     3684      +23     
+ Misses       1890     1857      -33     
+ Partials       10        9       -1     
Impacted Files Coverage Δ
...oscope-flamegraph/src/SharedQueryInput.module.scss 61.54% <ø> (ø)
packages/pyroscope-flamegraph/src/Toolbar.tsx 85.33% <85.00%> (-1.04%) ⬇️
...ages/pyroscope-flamegraph/src/SharedQueryInput.tsx 64.52% <100.00%> (ø)
...kages/pyroscope-flamegraph/src/Toolbar.module.scss 61.54% <100.00%> (ø)
...h/src/FlameGraph/FlameGraphComponent/CheckIcon.tsx 71.43% <0.00%> (-28.57%) ⬇️
webapp/javascript/services/apps.ts 28.58% <0.00%> (-1.05%) ⬇️
.../javascript/redux/reducers/continuous/selectors.ts 55.36% <0.00%> (-0.78%) ⬇️
... and 9 more

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.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 22, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 501.7 KB (+0.02% 🔺) 10.1 s (+0.02% 🔺) 2.7 s (+4.24% 🔺) 12.7 s
webapp/public/assets/app.css 19.61 KB (-0.11% 🔽) 393 ms (-0.11% 🔽) 0 ms (+100% 🔺) 393 ms
webapp/public/assets/styles.css 9.6 KB (0%) 192 ms (0%) 0 ms (+100% 🔺) 192 ms
packages/pyroscope-flamegraph/dist/index.js 129.92 KB (+0.06% 🔺) 2.6 s (+0.06% 🔺) 1.1 s (-42.9% 🔽) 3.7 s
packages/pyroscope-flamegraph/dist/index.node.js 130.63 KB (+0.04% 🔺) 2.7 s (+0.04% 🔺) 662 ms (+13.87% 🔺) 3.3 s
packages/pyroscope-flamegraph/dist/index.css 7.97 KB (-0.14% 🔽) 160 ms (-0.14% 🔽) 0 ms (+100% 🔺) 160 ms

@Ivanikitin Ivanikitin added the frontend Mostly JS code label Nov 22, 2022
@eh-am
Copy link
Collaborator

eh-am commented Nov 23, 2022

/create-server

@@ -33,8 +42,8 @@ export const TOOLBAR_MODE_WIDTH_THRESHOLD = 900;

export type ShowModeType = ReturnType<typeof useSizeMode>;

export const useSizeMode = (target: React.RefObject<HTMLDivElement>) => {
const [size, setSize] = React.useState<'large' | 'small'>('large');
export const useSizeMode = (target: RefObject<HTMLDivElement>) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also guys what do you think about this: if we have this "more" button functionality we can go towards removing size logic, where we have

large
Screenshot 2022-11-23 at 11 11 17

and small
Screenshot 2022-11-23 at 11 11 40

toolbars depending on width and just show large option with additional "more" button (3dot) ?

cc @eh-am @Rperry2174

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, that simplifies a lot!

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog dogfrogfog marked this pull request as ready for review November 28, 2022 16:29
@dogfrogfog
Copy link
Contributor Author

Screen.Recording.2022-11-28.at.17.32.22.mov

@Rperry2174
Copy link
Contributor

Looks great so far! dropdown gets hidden beneath the flamegraph:

Screen.Recording.2022-11-28.at.12.01.03.PM.mov

@Rperry2174
Copy link
Contributor

Also we currently drop groups starting on the right and moving to the left ... How hard would it be to reverse that so that the first items that disappear are the items on the left (i.e. head-first / tail first would disappear first)

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

Screen.Recording.2022-11-29.at.13.37.01.mov

@eh-am
Copy link
Collaborator

eh-am commented Nov 29, 2022

/create-server

@dogfrogfog
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.

love it! Thanks @dogfrogfog

@Rperry2174 Rperry2174 merged commit 78b27d8 into main Nov 30, 2022
@Rperry2174 Rperry2174 deleted the feat/add-collapsed-toolbar-2nd-row branch November 30, 2022 07:59
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

4 participants