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

Mobile and tablet layouts #2087

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Mobile and tablet layouts #2087

wants to merge 22 commits into from

Conversation

benjaminleonard
Copy link
Contributor

@benjaminleonard benjaminleonard commented Mar 21, 2024

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.

Copy link

vercel bot commented Mar 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Sep 16, 2024 8:15pm

@benjaminleonard
Copy link
Contributor Author

Priority for feedback on this PR will be ensuring we haven't regressed the desktop version.

<Button
variant="ghost"
size="sm"
className="md-:hidden"
Copy link
Contributor Author

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')
Copy link
Contributor Author

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.

{transitions(
({ x }, item) =>
item && (
<Dialog.Root
Copy link
Contributor Author

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.

)
}

export const ProfileLinks = () => {
Copy link
Contributor Author

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.

</div>
)
}

const SignOut16Icon = () => (
Copy link
Contributor Author

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

)
}

const Menu12Icon = ({ className }: { className: string }) => (
Copy link
Contributor Author

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


return useStore(
(store) => ({
isOpen: size.width >= 1024 ? true : store.isOpen,
Copy link
Contributor Author

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]`
Copy link
Contributor Author

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;
Copy link
Contributor Author

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' },
Copy link
Contributor Author

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.

@@ -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')
Copy link
Contributor Author

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

@david-crespo
Copy link
Collaborator

Obviously we need to test it properly, but I just tried it on my phone and it feels incredible.

@benjaminleonard
Copy link
Contributor Author

Refactoring the dialog out on desktop, its causing issues with side modals.

@benjaminleonard
Copy link
Contributor Author

No idea why this test is suddenly failing and it passes on my local machine.

@david-crespo
Copy link
Collaborator

Couple things.

  1. I'm not sure why it suddenly became reproducible locally after I merged main. Could be that I re-ran npm install
  2. The failure here was simply that the element in was very slightly in the viewport (not sure why that changed). The error message from playwright about that happens to be very opaque.
  3. The fix is to scroll 5px farther.
image

<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">
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image image

This seems to be fine because all of the popovers are in portals and therefore appears later in the DOM. I could add a z-bottomBar or something that is z-index: 5; (popover is currently 10).

@benjaminleonard
Copy link
Contributor Author

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.

@david-crespo
Copy link
Collaborator

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.

@benjaminleonard
Copy link
Contributor Author

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.

@david-crespo
Copy link
Collaborator

I merged main and it all seems to work, so we're in better shape than I expected. Here's one easy thing to fix — needs a max-w or a slightly smaller hard width.

image

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]">
Copy link
Collaborator

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

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 this pull request may close these issues.

2 participants