-
Notifications
You must be signed in to change notification settings - Fork 12
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
Mobile and tablet layouts #2087
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Priority for feedback on this PR will be ensuring we haven't regressed the desktop version. |
<Button | ||
variant="ghost" | ||
size="sm" | ||
className="md-:hidden" |
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 be disabled universally I think. Users will not be using the CLI from their phones.
@@ -29,10 +29,18 @@ function ExternalLink({ href, children }: { href: string; children: ReactNode }) | |||
export function MswBanner() { | |||
const [isOpen, setIsOpen] = useState(false) | |||
const closeModal = () => setIsOpen(false) | |||
|
|||
useEffect(() => { | |||
document.body.classList.add('msw-banner') |
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.
Perhaps convoluted but only appears on the preview so not too concerned about it. We use it to add extra padding for the banner at the bottom. Banner has been moved to the bottom, mostly because it breaks less stuff in the navigation.
app/components/Sidebar.tsx
Outdated
{transitions( | ||
({ x }, item) => | ||
item && ( | ||
<Dialog.Root |
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.
Feels slightly weird to be using a dialog here but it means we can use the same sidebar across both layouts. If there's a place that needs a bit more of a discerning eye it's probably here and the state logic to make sure it's bulletproof.
app/components/Sidebar.tsx
Outdated
) | ||
} | ||
|
||
export const ProfileLinks = () => { |
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.
Adds profile info into the navigation, hidden on desktop because it's visible in the nav.
app/components/Sidebar.tsx
Outdated
</div> | ||
) | ||
} | ||
|
||
const SignOut16Icon = () => ( |
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'll move this into the design system
app/components/TopBar.tsx
Outdated
) | ||
} | ||
|
||
const Menu12Icon = ({ className }: { className: string }) => ( |
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.
Likewise, will also add to the design system
app/hooks/use-menu-state.ts
Outdated
|
||
return useStore( | ||
(store) => ({ | ||
isOpen: size.width >= 1024 ? true : store.isOpen, |
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 tend to not like to target window sizes with javascript, but this felt like the easiest option. Forces the sidebar to be open on larger screen sizes since it's always visible.
export const PageContainer = classed.div`grid h-screen grid-cols-[14.25rem,1fr] grid-rows-[60px,1fr]` | ||
export const PageContainer = classed.div`h-full pt-[var(--navigation-height)] [overscroll-behavior-y:none]` |
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.
Switched back to good old salt of the earth non-grid CSS. It was a great solution but we ran into some awkward browser behaviour where a forced scroll would mess up the layout irredeemably.
@@ -44,11 +44,34 @@ | |||
|
|||
:root { | |||
--content-gutter: 2.5rem; | |||
--sidebar-width: 14.25rem; | |||
--navigation-height: 3.75rem; |
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.
We should do more of this stuff I think
'xl-': { max: '1535px' }, | ||
'lg-': { max: '1279px' }, | ||
'md-': { max: '1023px' }, | ||
'sm-': { max: '767px' }, | ||
'sm+': { min: '640px' }, |
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 never used this -
/ +
for responsive Tailwind before and I must say I am a fan, means we're not forced to class them mobile first.
test/e2e/scroll-restore.e2e.ts
Outdated
@@ -8,13 +8,13 @@ | |||
import { expect, test, type Page } from './utils' | |||
|
|||
async function expectScrollTop(page: Page, expected: number) { | |||
const container = page.getByTestId('scroll-container') | |||
const container = page.locator('#content_pane') |
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 was adding an ID to content pane already, so we no longer need the test-id
Obviously we need to test it properly, but I just tried it on my phone and it feels incredible. |
Refactoring the dialog out on desktop, its causing issues with side modals. |
No idea why this test is suddenly failing and it passes on my local machine. |
<SkipLinkTarget /> | ||
<main className="[&>*]:gutter"> | ||
<Outlet /> | ||
</main> | ||
</div> | ||
<div className="sticky bottom-0 flex-shrink-0 justify-between overflow-hidden border-t bg-default border-secondary empty:border-t-0"> | ||
<div className="sticky bottom-0 z-popover flex-shrink-0 justify-between overflow-hidden border-t bg-default border-secondary empty:border-t-0"> |
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.
this might conflict with actual popovers
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.
Would love to get this into release 8 – in part because it touches so many files the longer it sits the more painful any merge conflicts are going to be. |
This is beautiful work but because it's hard to say it's a priority and it has so many moving parts (and is getting more and more out of date), I think we'll want to break it into chunks (as far as that's possible) and redo each one as a new PR on top of main. |
The important part here is fixing the parts that break on smaller desktop sizes currently. And removing the grid layout is necessary I think for fixing the scrolling bug on the instance create (and other screens) – which is probably the most invasive change. Happy for this to be split out, though I'd be doubtful that would be less work than spending a bit of time testing this. If we deployed this as early on the dogfood cycle before the next release, and did some extra checks on our end then the scope for harm is pretty low. Especially since our e2e coverage is pretty good. |
return ( | ||
<> | ||
<div className="flex items-center border-b border-r px-3 border-secondary"> | ||
<div className="fixed top-0 z-topBar col-span-2 grid h-[var(--navigation-height)] w-full grid-cols-[min-content,auto] bg-default lg+:grid-cols-[var(--sidebar-width),auto]"> |
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.
this stuff freaks me out. wonder if we can drop the grid cols thing
Long overdue, but not as painful as I had expected. There are definitely refinements to be made to mobile layouts, but this is a good start. Feels a little magic to create an instance so easily and connect to it from your phone.
At some point we'll want to think about the actions that people are more likely to take on mobile devices and optimise for that. I'm thinking about dashboards, fault management, checking and increasing utilization – aka someone has pinged me about something and as an operator I need to check it out.
Some things are not disabled but unlikely to be usable on a mobile device. For example uploading an image – props to the first person to successfully upload a raw bootable image and boot from it.