UI: Mobile Views #4536
UI: Mobile Views #4536
Conversation
There was a problem hiding this comment.
I'll let someone else do the JS side, but design-wise this looks really nice. I did a similar approach in Vault (
https://github.com/hashicorp/vault-enterprise/pull/240) but have never been happy with the position or controls. I like that the control stays in the same spot and that light logo is super nice. I might steal this 👍
| transform: translate(-35%, 0%); | ||
| height: 100%; | ||
| fill: desaturate(lighten($nomad-green, 20%), 30%); | ||
| cursor: pointer; |
There was a problem hiding this comment.
Any thoughts on using a style more similar to Terraform's icon they use here? The usage is different, but I like that the icon is pretty minimal
| padding: 1rem; | ||
| transform: translate(-35%, 0%); | ||
| height: 100%; | ||
| fill: desaturate(lighten($nomad-green, 20%), 30%); |
There was a problem hiding this comment.
It'll be really nice to have the full palette soon :\
ui/app/styles/components/gutter.scss
Outdated
| width: $gutter-width; | ||
| transform: translateX($gutter-width); | ||
| z-index: $z-gutter; | ||
|
|
There was a problem hiding this comment.
Yes and no? I remember thinking it made the code read better, particularly the part where the display is none despite all those other styles being there.
Looking at it again now, I don't find it helpful. Will remove.
|
In my opinion, anything "acceptable" 🙌 is better that "totally does not work"; for that late night call when you have to debug something serious! 😁 |
There was a problem hiding this comment.
Hey,
I think this is a super important addition 👍 , good work on whats here. Couple of little things I noticed.
What I see doesn't look exactly like your screengrabs, I tried it in a few different browsers just incase but this is what I see:
Notice the hamburger is a little snug up to the logo, and possibly lined up weirdly with whats underneath. Whilst thats not super important for a merge, it might be affecting the second thing I noticed.
It looks like only half the hamburger icon is clickable? Looks like its half covered by the logo?
I'd probably sort that before merging.
Just noticed the menu does some weird 'appear then quickly tween out' thing when you change the size of your browser window thing, defo not a blocker that one tho.
I've not really reviewed the code for this as it looks like a lot of the previous PR. Just wondering if its worth PR'ing this against your previous branch instead of master, would that only show the changes here and not the previous PR also? It's a little tough trying to follow what's changed all at once? Once they are both approved you could merge them down on top of each other and then into master? I dunno, maybe there's some GitHub UI foo I don't know about 😄
The menu definitely no longer looks like the screenshots. I chatted with @captainill and @joshuaogle after opening this and the consensus was to make the hamburger more traditional. It shouldn't be snug against the logo like you noted, so I'll look into that. It also shouldn't have it's click area eclipsed by the logo, so I'll definitely look into that. Thanks for the tire-kicking!
Definitely makes this hard to review, which I called out in the description. The plan is:
|
DingoEatingFuzz
force-pushed
the
f-ui-mobile-views
branch
from
34697f8
to
ce117bf
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Builds on #4533, rebase after that merges
I would call these "mobile acceptable", not "mobile friendly".
We still don't prioritize using the UI on a phone, but hey, sometimes you gotta do what you gotta do.
The breakpoint for the flyout menu is at 960px, so it will also help people who don't use their whole screen for their browser.