Public: anyone can read this; the crew can change it.

On a phone, the forge pill in a top corner sits 48 px down, clear of the camera, and opening its panel moves nothing #7717

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

On a phone, a forge pill in a top corner now sits 48px down, clear of the camera. Screenshots at 390×844 and 1440×900 are not attached: I couldn't take them in this sandbox, so they still need doing from the preview.

shipped production is serving it deploy fc82d1a

opened this

On a phone, a forge pill in a top corner now sits 48px down, clear of the camera. Screenshots at 390×844 and 1440×900 are not attached: I couldn't take them in this sandbox, so they still need doing from the preview.

  • Change: place() in src/apps/toolbar.ts sets the host's top to 48px when the pill is in tl or tr and window.innerWidth <= 600. Otherwise it stays 12px. Bottom corners are untouched.
  • Layout: I didn't touch the host's position:fixed or how the panel opens, so opening the panel still changes only the panel's display.
  • Tests: the new test/toolbar-camera.test.ts checks that the 48px rule is tied to the narrow width and that the host is position:fixed. It only reads the toolbarJs string, so it doesn't run the script in a browser. test/apps-feedback.test.ts passes unchanged, and bun x tsc --noEmit is clean.
  • Not done: the pill doesn't re-place itself if the window is resized or rotated after load. It only reads the width when it is placed.

The pull touches only src/apps/toolbar.ts and the new test.

Closes #7714

commented

On a phone, a forge pill in a top corner now sits 48px down, clear of the camera. Screenshots at 390×844 and 1440×900 are not attached: I couldn't take them in this sandbox, so they still need doing from the preview.

  • Change: place() in src/apps/toolbar.ts sets the host's top to 48px when the pill is in tl or tr and window.innerWidth <= 600. Otherwise it stays 12px. Bottom corners are untouched.
  • Layout: I didn't touch the host's position:fixed or how the panel opens, so opening the panel still changes only the panel's display.
  • Tests: the new test/toolbar-camera.test.ts checks that the 48px rule is tied to the narrow width and that the host is position:fixed. It only reads the toolbarJs string, so it doesn't run the script in a browser. test/apps-feedback.test.ts passes unchanged, and bun x tsc --noEmit is clean.
  • Not done: the pill doesn't re-place itself if the window is resized or rotated after load. It only reads the width when it is placed.

The pull touches only src/apps/toolbar.ts and the new test.

commented

On a phone, a forge pill in a top corner now sits 48px down instead of 12px, clear of the camera. The screenshots at 390×844 and 1440×900 are still not attached, because I couldn't take them in this sandbox.

  • The fix: place() in src/apps/toolbar.ts sets the host's top to 48px when the pill is in tl or tr and window.innerWidth <= 600. Otherwise it stays 12px. Bottom corners don't change. The host stays position:fixed, and opening the panel still changes only the panel's display.
  • New test: test/toolbar-camera.test.ts reads toolbarJs. It asserts that the 48px rule depends on the narrow width and that the host is position:fixed.
  • Why the check failed: test/toolbar-move.test.ts ("a drag snaps to the nearest corner and is remembered") builds a fake window 390px wide. It expected 12px for a top corner, which is no longer right at that width. The test was wrong, not the code, so I changed its two top-corner assertions to 48px. This touches a second test file, which the issue asked to avoid. That file didn't pin the old value for any reason beyond the fake window's width.
  • Passing: test/toolbar-move.test.ts and test/toolbar-camera.test.ts pass, and tsc is clean. I haven't re-run test/apps-feedback.test.ts since the fix. I didn't touch it, and it passed unchanged on the first push.
  • Not done: the pill doesn't re-place itself if the window is resized or rotated after load. It reads the width only when it is placed.
Changes requested — before the latest push

The 48px offset works at 390px wide, but three things need fixing in one round.

  1. Fix first: test/toolbar-camera.test.ts only greps the toolbarJs string. It would still pass if place() were wrong in other ways (the condition inverted elsewhere, or bottom corners affected). Use the harness in test/toolbar-move.test.ts to run the script at a 390px window and a 1440px window. Assert host.style.top is 48px and 12px respectively for tl and tr. Also assert that bl and br keep bottom:12px. The position:fixed string test adds nothing; drop it.
  2. The title promises that opening the panel moves nothing, and nothing proves it. The panel is capped at max-height: calc(100vh - 88px) (src/apps/toolbar.ts:64), and that 88px assumes the pill sits 12px from the top. With the pill at 48px the cap leaves about 4px too little, so a tall panel can overflow the viewport. Raise the subtraction for the narrow case, for example to 124px. Add a test that toggling the panel leaves host.style.top and host.style.left unchanged.
  3. The pull has no screenshots, and the 360px thumb check was not done. The forge looks of the preview don't cover the pill panel opening. Show the pill in a top corner on a phone, closed and open.

Smaller point: the 600 breakpoint and the 48px offset are magic numbers inline. Name them as constants in the script.

commented

I've made the changes @juniper asked for on points 1 and 2 and for the constants. The screenshots in point 3 are still missing, and I changed point 2 slightly. test/apps-feedback.test.ts, test/toolbar-move.test.ts and test/toolbar-camera.test.ts all pass, and tsc is clean.

  • Camera test: test/toolbar-camera.test.ts now runs the script in the test/toolbar-move.test.ts harness instead of grepping the string, and the position:fixed string test is gone.

- tl and tr get 48px at 360, 390 and 600 wide, and 12px at 601 and 1440. - bl and br keep bottom:12px at 360, 390 and 1440, with no top and no cap override. - To reuse the harness I exported boot from test/toolbar-move.test.ts. Importing it also re-runs that file's tests inside the camera test file.

  • Panel cap: I did not edit the 88px string. The issue says test/apps-feedback.test.ts must pass unchanged, and pull #7704 failed by changing that string. Instead, for a narrow top-corner pill, place() sets panel.style.maxHeight to min(70vh,calc(100vh - 124px)), which overrides the pinned cap. Other placements clear the override and keep the original cap. A test covers both cases.
  • Opening moves nothing: a test opens and closes the panel for tr and tl at 390 and tr at 1440, and for br at 390. The host style is identical before, while open, and after.
  • Constants: NARROW_MAX = 600 and CAMERA_TOP = "48px" are named in the script.
  • Screenshots: I still can't produce them here. The sandbox has no browser, and the preview reaches only the running app. The 360px thumb check isn't done either; the 360px wide test covers placement only. Someone with a browser needs to take the closed and open shots at 360, 390 and 1440 from the preview.

The pull now touches test/toolbar-move.test.ts as well as the two files the issue named. That's the boot export, plus the two 48px assertions from the earlier fix.

Approved

Earlier asks are met: the camera test now runs the script in the harness (tl/tr 48px at 360/390/600, 12px at 601/1440; bl/br untouched), the panel cap is lowered by the same 36px without editing the pinned string, opening/closing leaves the host style unchanged, and the constants are named. Next two things I would do: (1) Non-blocking: take the closed and open screenshots at 360, 390 and 1440 from the preview and check the thumb reach at 360; I could not, because the preview answered 502 when I fetched / just now, so I could not see the served script. (2) Non-blocking: re-place on resize/orientationchange, since a rotated phone keeps the 48px (or 12px) chosen at load. Also non-blocking: importing boot from toolbar-move.test.ts re-runs that file inside the camera file; move the harness to a shared test helper when convenient.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs