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

Sidebar fixes #1497

Merged
merged 12 commits into from
Jul 31, 2024
Merged

Sidebar fixes #1497

merged 12 commits into from
Jul 31, 2024

Conversation

bhousel
Copy link
Contributor

@bhousel bhousel commented Jul 31, 2024

This fixes a few known issues with the sidebar
#1468
#1479

While in the sidebar code I did a bunch of other cleanups:

  • Added a proper resizer handle
  • Fix how resizing works (see below)
  • Fix a handful of RTL (right to left) issues.
  • Flexbox everything in this part of the UI : container, sidebar, main content, inspector and other sidebar content
    (this allows us to much easier keep track of how wide the user actually wants the sidebar to be)
  • Update some of this code to class components (I'm finding this style of code a lot easier to reason about as to when they are created, when they render, etc than our old idiomatic D3 UI code)

OLD:
old resize

NEW:
new resize

I think the postcss autoprefixer plugin uses this information when proccessing
our css file, and I'm trying to get it to avoid adding as many old rules.
This modernizes the components to be written in a way that we can more easily
support rerendering pieces of the app.

Flexbox also simplifies the sidebar calculations a lot, and should
help a lot with mobile and localization (e.g. ltr-rtl)

I'm also trying out a few new naming conventions that might help
us work with our (admittedly very confusing and complex) UI code:
- capital names for components (like React).      `inspector`    -> `Inspector`
- dollar prefix for d3-selections (like JQuery)   `sidebar`      -> `$sidebar`
- double dollar signs for _enter_ selections      `sidebarEnter` -> `$$sidebar`
summary:
- dragging sidebar resizer works
- toggling in/out by clicking resizer works
- persist sidebar preferences to local storage (closes #1479)
- fix "viewport jumps after toggling sidebar" (closes #1468)
  (though the resizing is still jerky as it fights with map draws and I'd like to improve that.)
- a few places rename systems to use shortnames (e.g. photosystem -> photos)
- we don't need utilFastMouse anymore

todo:
- I left the sidebar resizer red - need to style it nicer
- Remove that "Inspect" button?  It's unnecessary with a proper resizer
- ui.resize doesn't pan the map completely correctly to account for the resize
  I divided pan amount by half, expecting that the map will grow on both sides (from center) now
  But this still isn't totally right - the math needs to be based on main-map dims not sidebar dims.
  (for fun, this effect of weird pan is worse when the map is rotated)
Fixes are mostly
- Don't add a `dir` attribute anywhere except at the top level (container)
- Prefer `display: flex` where possible, as it respects that attribute.
  (aka avoid floated content, or special css rules to target `dir=rtl`)
Mentioned in 75ff758
Resizing (either the window or the sidebar) has been pretty janky and I'd like to improve this.
This seems to happen for a few reasons
- During a resize, the map dimensions, pixi dimensions, and temp transform of supersurface
  don't all agree.  Trying to redraw when these are not agreeing causes jumpiness
- I'm now classing the container as 'resizing' when the user is actively
  resizing something, we can use this information to avoid changing the
  pixi/canvas dimensions until the resize has settled.
- Starting to track the difference between the canvas and map dimensions ("drift"?)
  and hopefully we can account for it by sliding the supersurface around.
  (to keep the map settled in one place until an actual redraw happens)
Now
- `UiSystem.resize` will better handle the resizing to avoid having the map
  jump around during resizes.
- `PixiRenderer` will avoid changing the transform between full draws and
  better manage the temporary transform set on the supersurface
Because resizing blanks the canvas, it's better to do it immediately before we
instruct Pixi render to it.

The old code did resize in APP, which meant that after resizing,
the canvas would flash "black" until the next animation frame.
@bhousel
Copy link
Contributor Author

bhousel commented Jul 31, 2024

BTW I don't really like this "inspect" button. If users can click on the resizer to toggle it, we don't need an extra button for this. I am also imagining a UI that works better for mobile or vertical screen orientations, where the drawer slides in from the bottom, not the side. Now that the whole thing is flexboxed, it doesn't need to be on the side necessarily.

Does anyone have an opinion on whether the button should stay or go?

Screenshot 2024-07-31 at 2 46 01 PM

toggle

@bhousel bhousel added this to the v2.4 milestone Jul 31, 2024
@bhousel bhousel requested a review from Bonkles July 31, 2024 19:38
@Bonkles
Copy link
Contributor

Bonkles commented Jul 31, 2024

I, too, despise the inspect button.

My thoughts:

  • Remove it for desktop. You've already replicated its toggle functionality when the user clicks-and-releases the innermost edge of the sidebar, so I don't see the value that the inspect button provides.
    • Is there some way we could make the sidebar expand shortcut more discoverable without having to hunt for it in the keyboard shortcuts dialog?

-For mobile, we should probably include something for the user to click on to expand/contract, as edge-of-screen taps/gestures are really hard to do / already overloaded on a lot of phones/tablets.

@bhousel
Copy link
Contributor Author

bhousel commented Jul 31, 2024

-For mobile, we should probably include something for the user to click on to expand/contract, as edge-of-screen taps/gestures are really hard to do / already overloaded on a lot of phones/tablets.

Now that we're using flexbox for layout, mobile could look like this. A thing I made in about 3 minutes just playing with a few CSS properties in the Chrome dev tools by switching flex-flow from row to column and a few other tweaks.

Screenshot 2024-07-31 at 4 43 08 PM

@bhousel
Copy link
Contributor Author

bhousel commented Jul 31, 2024

It actually works pretty well 😎
It needs work but it's better than having it stuck on the side.

rapid mobile drawer

@Bonkles
Copy link
Contributor

Bonkles commented Jul 31, 2024

I would say yeah go with that- but pick whichever axis is longer to do the accordion thing (say, if the user has their phone on its side, we should absolutely change the flex flow and collapse left-right)

Copy link
Contributor

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

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

solid, proper, grade A.

height: 35px;
.rapid-inspector {
display: flex;
flex: 1 1 0px;
Copy link
Contributor

Choose a reason for hiding this comment

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

💪

(re: #1497)

This also quietly changes a few of the text strings that we use to talk about this component.
"sidebar" -> "feature inspector"
Because if we move it to some other part of the screen like the bottom, it's not really a sidebar anymore
@bhousel
Copy link
Contributor Author

bhousel commented Jul 31, 2024

  • Remove it for desktop. You've already replicated its toggle functionality when the user clicks-and-releases the innermost edge of the sidebar, so I don't see the value that the inspect button provides.

    • Is there some way we could make the sidebar expand shortcut more discoverable without having to hunt for it in the keyboard shortcuts dialog?

Cool, yeah I agree totally..

  • I removed the toggle button and moved the tooltip to the sidebar resizer to make the shortcut discoverable.
  • I made the tooltip open on the sidebar side (so it would avoid popping over the map while the user is doing map stuff).
  • I also made it hide when the user starts resizing.

resizer tooltip

@bhousel bhousel merged commit fd4bd27 into main Jul 31, 2024
4 checks passed
@bhousel bhousel deleted the sidebar_fixes branch July 31, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants