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

Update the web user interface aesthetically #263

Merged
merged 11 commits into from
Nov 21, 2017
Merged

Update the web user interface aesthetically #263

merged 11 commits into from
Nov 21, 2017

Conversation

tdewolff
Copy link
Contributor

Update the web user interface aesthetically.
The Refine menu is greyed-out for pages besides the Graph.
The description box now opens upon clicking the filename in the top-right.

@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

Merging #263 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
+ Coverage    65.2%   65.23%   +0.03%     
==========================================
  Files          34       34              
  Lines        7174     7212      +38     
==========================================
+ Hits         4678     4705      +27     
- Misses       2102     2113      +11     
  Partials      394      394
Impacted Files Coverage Δ
internal/report/source.go 80.5% <100%> (-0.28%) ⬇️
internal/driver/webhtml.go 100% <100%> (ø) ⬆️
internal/measurement/measurement.go 33.64% <0%> (-1.86%) ⬇️
internal/report/report.go 30.41% <0%> (-1.09%) ⬇️
internal/driver/commands.go 43.5% <0%> (-1%) ⬇️
internal/binutils/binutils.go 57.44% <0%> (-0.71%) ⬇️
internal/graph/dotgraph.go 91.21% <0%> (+1.21%) ⬆️

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 0ade723...ed72f73. Read the comment docs.

@aalexand
Copy link
Collaborator

Some tests keep failing - related to the changes?

@tdewolff
Copy link
Contributor Author

@aalexand I don't think so, the fails seem to be in other places.

@aalexand
Copy link
Collaborator

I tried the updated UI styling checking out your branch, the new style is super super nice! Very slick.

I am looking into what makes the test failing on the master. I think the changes are unrelated too.

@josef-jelinek can you look at this PR from CSS / JS point of view please? The changes look OK to me, but one more LGTM from you would be useful.

@josef-jelinek
Copy link
Contributor

It looks good to me, just the indentation changes could have been in a separate PR, since there are many falsely detected changes now increasing a probability of an issue slipping through.

@aalexand aalexand changed the title WebUI Update the web user interface aesthetically Nov 18, 2017
@aalexand
Copy link
Collaborator

Can you rebase the PR onto current master? I expect that to fix the CI.

@tdewolff
Copy link
Contributor Author

@josef-jelinek I'll keep this in mind next time
@aalexand Tests are fixed now

Copy link
Collaborator

@aalexand aalexand left a comment

Choose a reason for hiding this comment

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

One nit comment, please fix, I'll merge then.

@@ -14,234 +14,291 @@

package driver

import "html/template"
import (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is not needed, please revert.

@tdewolff
Copy link
Contributor Author

I think that was goimports, but it didn't do it on save this time.

@aalexand
Copy link
Collaborator

@tdewolff @spiermar Both #188 and #263 are about to land now, and I think there are going to be some merge conflicts for one of you, as an FYI. I think most of those could be avoided if we defer the whitespace updates in #263, but not sure if it's worth it. Thoughts?

@tdewolff
Copy link
Contributor Author

I can undo the ws changes, they are not important.

@aalexand
Copy link
Collaborator

aalexand commented Nov 21, 2017 via email

@aalexand aalexand merged commit 71d5bad into google:master Nov 21, 2017
@tdewolff
Copy link
Contributor Author

Nice! Might be best if I make a PR after #188 to refactor and indent all code properly before pursuing other changes.

@aalexand
Copy link
Collaborator

@tdewolff Yep, it probably makes sense to wait un poquito.

aalexand added a commit to aalexand/pprof that referenced this pull request Dec 20, 2017
Pull request google#263 disabled the refine menu for any views other than
Graph, but that's not expected - it's perfectly fine to use it for Flame
and Top, so just keep it enabled.

Fixes google#285.
aalexand added a commit that referenced this pull request Dec 22, 2017
Pull request #263 disabled the refine menu for any views other than
Graph, but that's not expected - it's perfectly fine to use it for Flame
and Top, so just keep it enabled.

Fixes #285.
lannadorai pushed a commit to lannadorai/pprof that referenced this pull request Feb 13, 2018
Pull request google#263 disabled the refine menu for any views other than
Graph, but that's not expected - it's perfectly fine to use it for Flame
and Top, so just keep it enabled.

Fixes google#285.
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
Update the web user interface aesthetically.
The Refine menu is greyed-out for pages besides the Graph.
The description box now opens upon clicking the filename in the top-right.
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
Pull request google#263 disabled the refine menu for any views other than
Graph, but that's not expected - it's perfectly fine to use it for Flame
and Top, so just keep it enabled.

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

Successfully merging this pull request may close these issues.

5 participants