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: icicle layout #64

Merged
merged 5 commits into from
Dec 18, 2018
Merged

feat: icicle layout #64

merged 5 commits into from
Dec 18, 2018

Conversation

bahtiyarkaba
Copy link
Contributor

This PR attempts to resolve #8 by adding a dropdown to select the layout of the flame-graph, with "flame" or "icicle" options.

The icicle layout is drawn by setting inverted on the flame graph.

Dropdown is chosen as it will also be useful for/in case sunburst layout #45 would be incorporated as well. I plainly put the control next to reset button. We might need some cosmetic tweaking if it looks ugly.

The chosen layout is stored to localstorage for persistence.

@spiermar spiermar self-requested a review December 10, 2018 18:11
@spiermar spiermar self-assigned this Dec 10, 2018
Copy link
Contributor

@spiermar spiermar left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just suggesting a few small changes.

src/components/FlameGraph/FlameGraph.jsx Outdated Show resolved Hide resolved
src/components/FlameGraph/FlameGraph.jsx Outdated Show resolved Hide resolved
src/components/FlameGraph/FlameGraph.jsx Show resolved Hide resolved
src/components/FlameGraph/FlameGraph.jsx Outdated Show resolved Hide resolved
@bahtiyarkaba
Copy link
Contributor Author

@spiermar I included the proposed changes. Could you review?

@spiermar
Copy link
Contributor

@bahtiyarkaba will do

@spiermar spiermar merged commit 64eb9ca into Netflix:master Dec 18, 2018
@brendangregg
Copy link
Contributor

The d3 transition is really nice. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

icicle layout
3 participants