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

working on a new layout #16

Open
3 tasks done
JonathonDunford opened this issue Mar 27, 2018 · 7 comments
Open
3 tasks done

working on a new layout #16

JonathonDunford opened this issue Mar 27, 2018 · 7 comments

Comments

@JonathonDunford
Copy link
Member

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:

  • More robust layout in general with less jumping around and odd behavior on specific browsers (like the intermittent #102)
  • Responsive layout on narrow/mobile devices (now it renders each widget box on a screen at a time even on very small devices)
  • Divides the rows of widgets into equal parts to reduce jagged "lines" in the layout

TODO:

  • Font sizes somehow got bigger for some reason (trades, orderbook, volume)
  • New Order content is too wide, causing scrollbars
  • I believe there are a lot of style classes not used anywhere anymore. Remove them from black.css

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

@JonathonDunford
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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


Looks good to me!

@JonathonDunford
Copy link
Member Author

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!

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 a pull request may close this issue.

1 participant