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

React component to visualize gas price vs confirmation time #50

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@eswarasai
Copy link
Contributor

commented Jan 17, 2018

Fixes: #34

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2018

@owocki @danfinlay -- I've committed the code for the React Component to display the graph between Gas Price (X-Axis) vs Confirmation Time (Y-Axis with Log scale) which can be accessed in the frontend directory at /chart. I still need to tweak the design a bit, I probably might need a spec for this. Apart from that let me know if there's anything that I might've missed out or need to change. Thanks.

@danfinlay
Copy link

left a comment

The code looks good, I'll try to pull & build & use soon, but it would be even nicer if you could post a demo instance that we could try out.

@@ -10,12 +10,12 @@ class Info extends React.Component {
return (

This comment has been minimized.

Copy link
@danfinlay

danfinlay Jan 18, 2018

JSX files should have the suffix .jsx.

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2018

@danfinlay -- Sorry for the delay in getting back to you.
It's now live at here -- https://eswarasai.github.io/gasprice/

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

@owocki @danfinlay -- Any suggestions/feedback/modifications needed to be done here? Thanks

@danfinlay

This comment has been minimized.

Copy link

commented Jan 23, 2018

Wow, what a hard ramp down!

Maybe the chart should auto-scale to focus on the area where the transition is made: Cut off the prices that flatline (too high), so we can see the slope more granularly?

Sorry for the slow replies, things have been pretty crazy over here. This generally looks great, thanks for posting the sample page!

This chart is live as in it is using current block data?

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

@danfinlay -- Yes. I'm using live data directly from this endpoint : https://ethgasstation.info/json/predictTable.json

I can see what you're suggesting with the slope of the graph but not really sure how to get it done. Let me spend some time on it and get back to you. In case you have any suggestions on how to achieve this, I can go ahead and implement them right away. Thanks.

@danfinlay

This comment has been minimized.

Copy link

commented Jan 24, 2018

I would just imagine that before feeding the price points into the graph, you could filter off any points whose gas price is higher than the lowest priced point with the lowest confirmation time.

@danfinlay

This comment has been minimized.

Copy link

commented Jan 24, 2018

While this fine tuning is awesome, I think this chart & data source generally meets the criteria of the original bounty, @owocki, what do you think?

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 24, 2018

i agree.. we may want to add another bounty to make it a little prettier (and/or maybe merge it into metamask-extension in some way -- but thats your call @danfinlay )

@danfinlay

This comment has been minimized.

Copy link

commented Jan 24, 2018

Yeah, I think if we want to establish a prettiness standard with an issue, the original bounty should include a design for the developer to reference. It feels cruel to jerk a dev who's satisfied the basic criteria back and forth too much.

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

@danfinlay @owocki -- Based on the above comments, I feel the plan of action would be to first get a design spec done for this component, so that I can then pick it up to update the UI as well as to handle the filtering of gasprice data points. Feel free to add if I'm missing something here. Thanks.

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

Based on the above comments, I feel the plan of action would be to first get a design spec done for this component, so that I can then pick it up to update the UI as well as to handle the filtering of gasprice data points.

@danfinlay is there a go-to designer that yall work with that we can loop in here?

@danfinlay

This comment has been minimized.

Copy link

commented Jan 25, 2018

Our designs are generally from @cjeria, looping him in and linking him this thread.

@cjeria

This comment has been minimized.

Copy link

commented Jan 25, 2018

@danfinlay @owocki If i'm reading this issue thread correctly, the design request is to add some visual polish to this chart? https://eswarasai.github.io/gasprice/

@cjeria

This comment has been minimized.

Copy link

commented Jan 25, 2018

Wow, what a hard ramp down!

Maybe the chart should auto-scale to focus on the area where the transition is made: Cut off the prices that flatline (too high), so we can see the slope more granularly?

Sorry for the slow replies, things have been pretty crazy over here. This generally looks great, thanks for posting the sample page!

This chart is live as in it is using current block data?

Sounds like we may also want to add zoom controls or "range selector buttons" to cut off the flatlines?

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@cjeria -- Yes. We'd like to make the above chart visually appealing. There won't be any zoom controls or buttons. The data will be pre-processed on the client side to ensure that the slope in the chart is much clearer

FYI : I'm using Recharts library for the Chart component

@danfinlay

This comment has been minimized.

Copy link

commented Jan 25, 2018

For @cjeria, I think it would be smart to imagine this chart somehow embedded in an advanced tab of the new metamask UI, and follow that visual language.

@cjeria

This comment has been minimized.

Copy link

commented Jan 25, 2018

@eswarasai @owocki @danfinlay cool. I've already started thinking about the advanced tab in the new UI and a way to display this chart. Here's the prototype. UX feedback is also welcome! https://consensys.invisionapp.com/share/39FJ047A4CU#/274301828_MetaMascara_Mobile_-_Structured

@danfinlay

This comment has been minimized.

Copy link

commented Jan 25, 2018

For the lazy, here is the relevant screenshot, which could already serve as the design for this graph!

screen shot 2018-01-25 at 3 20 33 pm

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

beauty.... @danfinlay is that relatively finalized? we could work off that

@danfinlay

This comment has been minimized.

Copy link

commented Jan 25, 2018

Yeah, I'd gladly approve the bounty if it looked like that.

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

@eswarasai see above. can you make it so?

ill throw in another 0.06 ETH (in addition to the staked 0.09 ETH) for the change in scope.

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@danfinlay @owocki -- Sure. I'll give it a try to get the Design as per the spec

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

@danfinlay -- Sorry for the delay in getting back on this. So I couldn't exactly match the UI from the mockup, but I atleast got to make it look better. Let me know your thoughts/feedback on this.
URL -- https://eswarasai.github.io/gasprice/

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2018

@danfinlay -- Any update on this? Thanks.

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

sorry for the radio silence @eswarasai -- @danfinlay and @vs77bb and i have been traveling

@danfinlay

This comment has been minimized.

Copy link

commented Feb 13, 2018

That looks pretty close, nice work @eswarasai!

A few notes:

  • Gas price should increase when moving from left to right
  • Gradient direction difference is ok, as long as colors match.
  • Would it be possible to get vertical lines in the chart, to mark price tiers?
@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2018

@danfinlay -- Thank you :)
As for your comments, please find the response below :

  • I've mimicked the Chart as per screens above. If I have to invert the gas price scale, then I'd have to mirror the entire Chart, so that it'll look better. Let me know if this is something you have in mind
  • I'll update the color code to match the spec
  • I've tried to get the vertical lines to mark the price tiers but couldn't with ReCharts library. I'll give it another shot and update here. This was something I was mentioning earlier which I couldn't match with the spec along with the Tooltip text
@danfinlay

This comment has been minimized.

Copy link

commented Feb 13, 2018

  • Yeah, sorry that the mockup had the x axis inverted, I think this was an oversight. We definitely want low on left and high on right.
  • Thanks
  • I'm ok not blocking over the vertical lines.
@cjeria

This comment has been minimized.

Copy link

commented Feb 13, 2018

@danfinlay yes and we may want to future proof this component as it may be used in a mobile context, so mobile responsive and touch sensitive is highly recommended.

See interaction animation study (pre-inverted version ixd study):
gas control animation

@eswarasai please let us know if you have any other questions!

@danfinlay

This comment has been minimized.

Copy link

commented Feb 13, 2018

I really like that animation @cjeria, also keep in mind we want the bottom axis to be gas price, going from low to high, and so the Y-axis will be confirmation time, and so the graph will start high and approach zero.

@cjeria

This comment has been minimized.

Copy link

commented Feb 14, 2018

@danfinlay correct. Here's an updated chart

time vs gas

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2018

@danfinlay -- I've done the above requested changes except for #3 i.e., the vertical lines. Let me know if there's anything else needed to modified -- https://eswarasai.github.io/gasprice/

@danfinlay

This comment has been minimized.

Copy link

commented Feb 21, 2018

I'd like a final confirmation from @cjeria here.

@cjeria

This comment has been minimized.

Copy link

commented Feb 23, 2018

@eswarasai See feedback notes in png

image

@danfinlay I also tested in mobile but doesn't appear to be responsive. In our UI, we're suggesting controlling the selected price position using the slider. We should try and put this through another phase of development to test the slider interaction.

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

@danfinlay @cjeria -- Sorry for complete radio silence on this one. So I've been trying to implement the changes suggested by Christian. There are few limitations that I cannot implement right away but few things can be tackled using work around though. I'll update the list here once I make the changes live. Thanks.

@owocki

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

mind articulating the limitations? we might be able to bring in someone to help

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

@owocki -- Please find the below limitations I've found so far :

  1. For the starting point of Axis, it cannot be 0, as the log scale doesn't have 0 value. So I need to create a DOM element and add custom CSS in order to handle the positioning etc
  2. Recharts support solid dots for active dot (on hover) and point dot for displaying the points on the line
  3. Cannot position info tooltip above the point, as it's getting calculated under the hood by the plugin. I've found a work around for this, but haven't really implemented it yet. Will do this and update here

These are the limitations for the current requirements. Although I don't have solution for solving 2 yet, but I'm confident that the other two points can be handled. Parallelly, keeping in mind the original design, I'm also trying to figure out a way to ensure that it's possible to implement the Slider as well with the current Recharts Component, rather than building it from the scratch using a different library

@owocki

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

thanks for the update.. i actually think @cjeria and @danfinlay are the folks you want to work with on the workarounds for these requirements as the finished code may make it their way eventually (not to gitcoin)

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2018

@danfinlay -- So I've just updated the tooltip UI/UX as per the above mockup which is live at here -- https://eswarasai.github.io/gasprice/

If you can let me know the priority/importance of above points 1 & 2, I'll try to get them done as well if possible. If not, I can start spending some time to get the desired Slider functionality implemented and get back to these minor UI changes later. Let me know how you'd like me to proceed. Thanks.

@PixelantDesign

This comment has been minimized.

Copy link

commented Apr 29, 2018

oOohhh love this!

@danfinlay

This comment has been minimized.

Copy link

commented Apr 30, 2018

Oh wow, that's a big upgrade!

Two other things that would be great:

  • Would it be possible to get a vertical line where the tooltip is?
  • Could we get an onClick handler, so the user could click this graph as their gas selection input?
@danfinlay

This comment has been minimized.

Copy link

commented Apr 30, 2018

Also might be cool to add the estimated time to the tooltip!

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

@danfinlay -- Yep. I think I can get the above 2 done. If I understand correctly, you'd like me to revert back to the old design where I had both the values like in the below screenshot?

It'd be great if I can get a mock with the updated tooltip design from @cjeria so that I can get it implemented :)

Screenshot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.