-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Change layout to bootstrap's grid system #923
Conversation
…ough properly on smaller screens yet
Ok, that should be a bit better. For the entities, the former sidebar content is now part of the main show The other links are pushed to the bottom appropriately. I think #924 might need to be done for the saves that work, as it's not even a failure on further testing but looks like the system is telling me off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one question/concern about some hard to understand code in one of the haml files, but it may not be applicable. I pulled the code locally, and checked it on my iPhone XR and it works like a champ. I clicked through everything and it seems to work without much trouble.
%tt #{t :assigned_to}: | ||
%dl | ||
%li | ||
%dt= pipeline != 0.0 ? number_to_currency(pipeline, precision: 0) : t(:n_a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use unless pipeline.zero? instead. might make the code more readable and ruby-esque
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I broadly just lifted and shifted existing code (here its mostly just adding a dl element; so it can be stuck into a flexbox model); but how about we put that into the rubocop rules/apply that globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've captured this one in #929 so we can do wider cleanup
Instead of that hamburger icon logo, could we have users click on the logo for ffcrm? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge effort. Great to see the screenshots. Thank you
Yeah, its currently at the point it 'works' but it's ugly, so keen to style it better. Raised #928 to really focus on the whole header more, particularly on mobile. |
Ok, I might merge this now with a view to start work on the improvements discussed this week. I'm pretty confident all of the feature tests actually pass; they are just flakey on CI - different failures for different runs. |
Fixes #465
Changes:
Before:
After:
Smaller device:
The sidebar now appears at the bottom:
Nav menu open (hamburger menu icon is a bit ugly still)