Skip to content
This repository has been archived by the owner on Nov 1, 2019. It is now read-only.

Add dynamic dimensions to comparison view graph, change x-axis scale #53

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Add dynamic dimensions to comparison view graph, change x-axis scale #53

merged 1 commit into from
Aug 30, 2018

Conversation

ecomerford
Copy link
Contributor

This PR addresses issues #43 and #29. I've added a window resize handler like the one @chutten made for the distribution view, so that now all three graphs respond to the window size and are much more readable.

I also changed the x_scale_type of the metrics-graphics graph to log, which seems to have fixed the shape of the graph, but for some reason made the lines disappear (you can still see each point when hovering). @wlach have you seen this happen before?

This PR may need a few more adjustments once I've looked into the log type and why it disappears the graph lines.

@wlach
Copy link

wlach commented Aug 29, 2018

@ecomerford I have not seen that, could you try to reproduce the issue with a minimal example on codepen (you can use this as a template: https://codepen.io/wlach/pen/LBvwLw) and file an issue with metrics graphics? It's probably just a bug.

handleChange = (event) => {
this.setState({compareVersion: event});
}

componentDidMount() {
window.addEventListener("resize", this.onResize);
this.onResize();
Copy link

Choose a reason for hiding this comment

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

With missioncontrol I used react-dimensions to scale the graphs automatically. I think it's a little more elegant and less bespoke:

https://github.com/mozilla/missioncontrol/blob/2e33078e75c096a63ca4c7cbb5854e1088ba3dee/frontend/ui/detailgraph.jsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, thanks! Reading up on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this runs into the same problem with the non-shown-but-still-rendered tabs where the containers have 0 height and width until shown, but I would be pleased to be shown to be wrong.

@ecomerford
Copy link
Contributor Author

screen shot 2018-08-29 at 2 02 00 pm

@wlach hm, I'm not able to reproduce it in that codepen example by switching the chart_type to line and x_scale_type to log. Here's a screenshot of the results of my updates in this PR, so you can see what I mean. The only thing in the code that I've changed is the width and the scale type.

Copy link
Collaborator

@chutten chutten left a comment

Choose a reason for hiding this comment

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

This is a fine step in the correct direction. We could merge this and take the react-dimensions investigation to a follow-up?

handleChange = (event) => {
this.setState({compareVersion: event});
}

componentDidMount() {
window.addEventListener("resize", this.onResize);
this.onResize();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this runs into the same problem with the non-shown-but-still-rendered tabs where the containers have 0 height and width until shown, but I would be pleased to be shown to be wrong.

@@ -73,6 +86,8 @@ export class ComparisonView extends Component {
y_accessor="proportion"
x_accessor="start"
area={[true, true]}
x_scale_type= "log"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like some unintentional whitespace after = on this and the following line?

@wlach
Copy link

wlach commented Aug 30, 2018

@ecomerford hmm, probably something about the shape of the data here then -- are you seeing any error in the console? If all else fails, if you give me instructions on how to check this out and test I could try to reproduce locally.

@ecomerford ecomerford merged commit e0fe748 into mozilla:master Aug 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants