-
Notifications
You must be signed in to change notification settings - Fork 2
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
Comments
Comment by activescott @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. |
Comment by freeatnet Definitely looks more even 👍 Some things I've noticed:
Looking forward to more! :) |
Comment by bricedurand It's much clearer! Also the price chart section is smaller than on master |
Comment by activescott @freeatnet Thanks for the feedback. Each addressed below:
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.
This exists in the old layout too, but I've pushed a fix :)
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).
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. |
Comment by activescott @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. |
Comment by bricedurand Looks good to me! |
Comment by activescott @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! |
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
The text was updated successfully, but these errors were encountered: