search

Agorex-io / FrontEnd

4 stars 2 forks source link

working on a new layout #23

Closed JonathonDunford closed 6 years ago

JonathonDunford commented 6 years ago

Issue by activescott Monday Feb 26, 2018 at 06:00 GMT Originally opened as https://github.com/forkdelta/forkdelta.github.io/pull/106

Please don't pull this yet until all todos below are resolved.

I'm sending this PR now to get feedback on the approach and to keep my status public.

This gets rid of a lot of the specific height settings that made the layout difficult to work with. Benefits are:

TODO:

activescott included the following code: https://github.com/forkdelta/forkdelta.github.io/pull/106/commits

JonathonDunford commented 6 years ago

Comment by activescott Saturday Mar 03, 2018 at 22:35 GMT

@freeatnet @JonathonDunford Would be great to get some eyes on this. I think this looks better and is more robust use of css/bootstrap layout than the prior. Should be easier to maintain going forward and better mobile/responsive support with less css.

If you guys can play with it on some different browses and devices and agree it is better pull it. If you'd like some changes make them or point me to them and I can investigate.

JonathonDunford commented 6 years ago

Comment by freeatnet Tuesday Mar 06, 2018 at 15:14 GMT

Definitely looks more even 👍

Some things I've noticed:

  1. Extra paddings below, above, or on the sides of element (order book, ticker, trades)
  2. Minor alignment issues (in Chrome, inputs vs buttons in "balances" pane)
  3. Text cut off in "balances" pane on smaller screen (e.g., Chrome's "Laptop with MDPI screen" responsive setting).
  4. The order in which pane widths are sacrificed on smaller screen: perhaps, we can cut the width of the trades column before we cut the width of ticker & balances.

Looking forward to more! :)

JonathonDunford commented 6 years ago

Comment by bricedurand Tuesday Mar 06, 2018 at 15:22 GMT

It's much clearer! However it might need clearer separations between sections. For exemple between Balance and Order Book sections, there isn't any space anymore. Before there was about 2px space, which makes it a bit more breathable.

Also the price chart section is smaller than on master

JonathonDunford commented 6 years ago

Comment by activescott Wednesday Mar 07, 2018 at 08:49 GMT

@freeatnet Thanks for the feedback. Each addressed below:

  1. Extra paddings below, above, or on the sides of element (order book, ticker, trades)

I did this deliberately as I thought it looked clearer. Actually to @bricedurand 's point, I was trying to make the separation between sections clearer. Happy to reduce or remove padding.

  1. Minor alignment issues (in Chrome, inputs vs buttons in "balances" pane)

This exists in the old layout too, but I've pushed a fix :)

  1. Text cut off in "balances" pane on smaller screen (e.g., Chrome's "Laptop with MDPI screen" responsive setting).

Good catch. There seems to be some resolutions that this column didn't fit into right on the edge of bootstrap's grid before bootstrap wraps the columns (to give the widget full screen width). I pushed a fix to set it up to scroll as needed and shrank the content as down as much as I could (without redoing the react code).

  1. The order in which pane widths are sacrificed on smaller screen: perhaps, we can cut the width of the trades column before we cut the width of ticker & balances.

I like this idea too, but I investigated and I don't see a clean way to get bootstrap to prioritize which column widths are sacrificed. I found a hack on stackoverflow but couldn't get it to work reliably myself (and it isn't supported by bootstrap officially so felt fragile).

Let me know what else you think needs done after your review.

JonathonDunford commented 6 years ago

Comment by activescott Wednesday Mar 07, 2018 at 08:59 GMT

@bricedurand Good one. I agree a subtle border between columns looks nice. So I've put that back in. Thanks for the feedback! Please check the latest and let us know if you think this is good to merge.

JonathonDunford commented 6 years ago

Comment by bricedurand Wednesday Mar 07, 2018 at 13:16 GMT

Looks good to me!

JonathonDunford commented 6 years ago

Comment by activescott Sunday Mar 11, 2018 at 21:30 GMT

@forkdelta/frontend-team I've merged the latest from master. I'd like to get this merged. If there is something holding it up please let me know!