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

fix: small toolbar fixes #1777

Merged
merged 8 commits into from
Dec 1, 2022
Merged

fix: small toolbar fixes #1777

merged 8 commits into from
Dec 1, 2022

Conversation

dogfrogfog
Copy link
Contributor

@dogfrogfog dogfrogfog commented Nov 30, 2022

Changes

  • fix console MMUI warning
  • fix tooltip on disabled item
  • add missing divider
  • return back divider margins
Screen.Recording.2022-11-30.at.13.35.05.mov

@dogfrogfog
Copy link
Contributor Author

/create-server

@github-actions
Copy link
Contributor

github-actions bot commented Nov 30, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 501.74 KB (+0.01% 🔺) 10.1 s (+0.01% 🔺) 4.6 s (+10.07% 🔺) 14.6 s
webapp/public/assets/app.css 19.66 KB (0%) 394 ms (0%) 0 ms (+100% 🔺) 394 ms
webapp/public/assets/styles.css 9.58 KB (0%) 192 ms (0%) 0 ms (+100% 🔺) 192 ms
packages/pyroscope-flamegraph/dist/index.js 129.96 KB (+0.03% 🔺) 2.6 s (+0.03% 🔺) 1.4 s (-0.12% 🔽) 4 s
packages/pyroscope-flamegraph/dist/index.node.js 130.68 KB (+0.04% 🔺) 2.7 s (+0.04% 🔺) 1.1 s (+41.91% 🔺) 3.7 s
packages/pyroscope-flamegraph/dist/index.css 7.97 KB (0%) 160 ms (0%) 0 ms (+100% 🔺) 160 ms

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Base: 66.30% // Head: 66.43% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (2071537) compared to base (084dd02).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1777      +/-   ##
==========================================
+ Coverage   66.30%   66.43%   +0.13%     
==========================================
  Files         170      170              
  Lines        5557     5548       -9     
  Branches     1260     1251       -9     
==========================================
+ Hits         3684     3685       +1     
+ Misses       1864     1854      -10     
  Partials        9        9              
Impacted Files Coverage Δ
...kages/pyroscope-flamegraph/src/Toolbar.module.scss 61.54% <ø> (ø)
packages/pyroscope-flamegraph/src/Toolbar.tsx 85.72% <100.00%> (+0.40%) ⬆️
.../javascript/redux/reducers/continuous/selectors.ts 64.45% <0.00%> (+10.06%) ⬆️

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.

@dogfrogfog dogfrogfog marked this pull request as ready for review November 30, 2022 12:36
@dogfrogfog dogfrogfog changed the title fix mui console warn fix: wrap disabled button in span Nov 30, 2022
@dogfrogfog
Copy link
Contributor Author

dogfrogfog commented Nov 30, 2022

added missing divider

Screen.Recording.2022-11-30.at.13.57.07.mov

@dogfrogfog
Copy link
Contributor Author

/create-server

Copy link
Contributor

@pavelpashkovsky pavelpashkovsky left a comment

Choose a reason for hiding this comment

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

lgtm

@dogfrogfog dogfrogfog changed the title fix: wrap disabled button in span fix: small toolbar fixes Nov 30, 2022
@eh-am
Copy link
Collaborator

eh-am commented Nov 30, 2022

For documentation purposes:
The error
image

Why are we adding a span https://mui.com/material-ui/react-tooltip/#disabled-elements

packages/pyroscope-flamegraph/src/Toolbar.tsx Outdated Show resolved Hide resolved
),
// sandwich view is hidden in diff view
width: TOOLBAR_SQUARE_WIDTH * (flamegraphType === 'single' ? 4 : 3),
width: TOOLBAR_SQUARE_WIDTH * (flamegraphType === 'single' ? 4 : 3), // 1px is to display divider
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just seeing this now, but it feels flaky...Ideally I would have 2 sets of const ToolbarItems = ToolbarItem[], one for single and another for double, then calculations would be derived from that.

(You don't have to act on it, I am just thinking out loud)

@pavelpashkovsky
Copy link
Contributor

pavelpashkovsky commented Nov 30, 2022

I approved this PR but just found that vertical margins for dividers are differing

image

dogfrogfog and others added 2 commits November 30, 2022 14:52
Co-authored-by: eduardo aleixo <eh-am@users.noreply.github.com>
@dogfrogfog
Copy link
Contributor Author

dogfrogfog commented Nov 30, 2022

I approved this PR but just found that vertical margins for dividers are differing

image

fixed
Screenshot 2022-11-30 at 15 06 35

@eh-am
Copy link
Collaborator

eh-am commented Nov 30, 2022

/create-server

@Ivanikitin Ivanikitin added the frontend Mostly JS code label Nov 30, 2022
@Rperry2174 Rperry2174 merged commit 196c6d8 into main Dec 1, 2022
@Rperry2174 Rperry2174 deleted the feat/fix-mui-error branch December 1, 2022 18:06
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.

5 participants