scrollOffset not respected

[rijk]

Hi Erik,

See the following Sandbox: https://codesandbox.io/s/react-laag-menu-viewport-margin-x40cc

When opening the menu, it has no margin relative to the viewport:

Even though scrollOffset: 12. Any idea what is the problem here?

[everweij]

Hey Rijk, Sorry for the late reply, I had a couple of days off (went to the Ardennes :)) Anyway, I'm afraid the behavior you describe is intentional, but by looking at your screenshot I get why it is a problem. Basically there's a constraint built into the code that says that the layer should never exceed the secondary anchor position. If we take your example, it means that for the anchor "LEFT_TOP", the layer should never be lower than the triggers top position. Now, I think most of the time this implicit behavior is fine, but I'm not sure whether your case is an edge case or not. For instance, one could argue that is aesthetically pleasing if the top of both the layer and trigger align horizontally (the reason I've built in this implicit behavior). You could use a dirty 'hack' and wrap your layer in an extra div, while applying a couple of pixels of padding on the top. What are your thoughts on this?

[rijk]

Hope you had a good time :) Writing this from Val Cenis 🏔

Thanks for the clarification.

For instance, one could argue that is aesthetically pleasing if the top of both the layer and trigger align horizontally

Unless this means the layer will touch the edge of the window, which doesn’t look aesthetically pleasing at all. So, maybe the scrollOffset should take precedence here (while keeping the existing alignment logic in place)? Doing it like that would even give the user a way to achieve the current behaviour if for some reason they want that; by setting scrollOffset to 0.

Right now, although your reasoning makes sense, it really feels like a bug to me. In my opinion I need to be able to trust the scrollOffset will always be respected.

[everweij]

Oh nice! Skiing or snowboarding? Had a great time in the Ardennes, plenty of small towns to visit, and the food was 👌🏻.

I hear you, somehow we need to make the scrollOffset and alignment logic fit together. A simplified overview of the strategy I'm currently thinking of:

1. Is autoAdjust enabled?
   • no, position layer with preferred anchor right away (respecting scrollOffset).
   • yes, goto 2.
2. Find best suitable anchor among possibleAnchors with triggerOffset and scrollOffset in mind. Constraint: When layerSide === ("left" || "right") layer.top should not > trigger.top, same applies when layerSide === ("top" || "bottom"). Does one anchor fit entirely?
   • yes, position layer with best suitable anchor right away.
   • no. Find the anchor with the biggest visible surface. Position the layer with this anchor (respecting scrollOffset)

To summarize:

• It's ok to search for the best possible anchor while satisfying the alignment logic stuff
• But when it comes down to the final positioning, the scrollOffset should be respected (when necessary)

How does this sound?

Edit: I should add that by using this adjusted strategy, there is a danger that the layer can become detached from the trigger. Therefore, I think, when layerSide === ("left" || "right") layer.top should never > trigger.bottom

[rijk]

Snowboarding ;)

Sound good, although I'm not sure if operation 2 would cause the menu to shift position? Anchor specification is as follows:

anchor: 'LEFT_TOP',
possibleAnchors: [
  anchor.RIGHT_CENTER,
  anchor.LEFT_CENTER,
  anchor.BOTTOM_CENTER,
  anchor.BOTTOM_LEFT,
  anchor.BOTTOM_RIGHT,
  anchor.TOP_CENTER,
]

I wouldn't want the layer to shift just because of the scrollOffset — although I could 'brute force' it by setting the possibleAnchors to [], as I know where the anchor is in this case.

Yeah, I am not even sure this is something that needs to be changed.. Need to think about it. The way I had it in mind was:

1. Find the best matching anchor
2. Position layer (with said top/bottom logic)
3. Apply scrollOffset if needed

So this way scrollOffset doesn't even influence the layer's position. But this maybe a little 'short through the turn' as we say huh ;D

[rijk]

Now that I thought about it some more, your way actually makes more sense. As the trigger is in the top right corner, the layer would not reposition I think (as e.g. BOTTOM_RIGHT would have the same scrollOffset consideration). It would reposition if say the trigger was in the center top of the page, to BOTTOM_CENTER, but this is actually what you want in this case.

[everweij]

Hey @Rijk, sorry it took me so long, but finally I had some time to look at this issue. I've just published v0.7.1. If you have time, let me know what you think ;)

[rijk]

Hah, and now I should apologize 🙃 Life huh. Hopefully all is well with you.

Not sure I understand correctly, but I updated the Sandbox to 1.8.0, and the layer is still glued to the top. Both with autoAdjust on and off.

[everweij]

Hey @rijk , Good to hear from you :)

Mmm that's a bummer! Well, good news is, is that I've finally released v2.0.0 today 🎉 I really hope you like the new 'style'!

In the playground of react-laag.com, I've quickly tried to reproduce your use-case:

Can you try upgrading to v2.0.0 when you have time? Sorry for the breaking changes btw, I hope migrating is a matter of minutes.

[rijk]

Hi Eric, wow! I wasn't sure if you were going to continue working on the package, but the new site looks amazing! The migration instructions are nice and clear also. Although the ToggleLayer component didn't bother me, I like the hooks based syntax as well. It does allow for more flexibility towards triggers, which in the previous version I sometimes had to wrap in a <span> to get it to work with custom components. I will try out the new version, and let you know if this new style works better in that regard.

Do you think it would be possible to reimplement ToggleLayer with the new hooks? That would make upgrading easier, and make the hooks opt in.

By the way, toying with the playground it seems there is a bug:

Maybe this is not a bug in the code, but just in code the playground generates; I've noticed that preferY is only added to the config when you select top:

So maybe the defaults should be the other way round because this doesn't seem to change anything.

[everweij]

Thanks, good to hear!

I created a very quick draft how you could 'translate' useLayer to ToggleLayer. I can't guarantee this will work 100%, but hopefully it will give you some sense of direction:

import * as React from "react";
import {
  useLayer,
  UseLayerOptions,
  LayerProps,
  LayerSide,
  UseLayerArrowProps
} from "react-laag";

type RenderLayer = (props: {
  layerProps: LayerProps;
  layerSide: LayerSide;
  arrowProps: UseLayerArrowProps;
  triggerBounds: ClientRect | null;
  isOpen: boolean;
  close: () => void;
}) => React.ReactNode;

type ChildRender = (props: {
  triggerRef: (element: HTMLElement | null) => void;
  isOpen: boolean;
  open: () => void;
  close: () => void;
  toggle: () => void;
}) => React.ReactNode;

type ToggleLayerProps = Omit<UseLayerOptions, "isOpen"> & {
  renderLayer: RenderLayer;
  children: ChildRender;
};

function ToggleLayer({ renderLayer, children, ...rest }: ToggleLayerProps) {
  const [isOpen, setOpen] = React.useState(false);

  function close() {
    return setOpen(false);
  }
  function open() {
    return setOpen(true);
  }
  function toggle() {
    return setOpen(!isOpen);
  }

  const {
    arrowProps,
    layerProps,
    layerSide,
    renderLayer: internalRenderLayer,
    triggerBounds,
    triggerProps
  } = useLayer({ ...rest, isOpen });

  return (
    <>
      {children({ triggerRef: triggerProps.ref, open, close, toggle, isOpen })}
      {internalRenderLayer(
        renderLayer({
          layerProps,
          layerSide,
          arrowProps,
          triggerBounds,
          isOpen,
          close
        })
      )}
    </>
  );
}

function Example() {
  return (
    <ToggleLayer
      renderLayer={({ isOpen, layerProps }) =>
        isOpen && <div {...layerProps}>Layer!</div>
      }
    >
      {({ triggerRef, toggle }) => (
        <button ref={triggerRef} onClick={toggle}>
          Toggle
        </button>
      )}
    </ToggleLayer>
  );
}

I've also updated the readme a bit to better explain the intention behind preferX / preferY -> in the options-table. Also creeated a dedicated section at 'concepts' explaining the strategy behind placement priorities.

[rijk]

Thanks. What I meant was to include a ToggleLayer component like this in the package. Should be doable, right?

[rijk]

I've also updated the readme a bit to better explain the intention behind preferX / preferY -> in the options-table.

Well, I still don't understand..

Reading all this, I still expect the layer to be at the bottom of the trigger, not the top 🤔

[rijk]

Oooooh now I understand. It's for these cases:

[rijk]

I would say the name is very confusing here. I would rather name it something like fallbackY / fallbackX or something?

[rijk]

And to be honest I still don't understand 😄

Isn't this the same effect as preferY: 'bottom'?

[rijk]

Has scrollOffset been removed by the way? The release notes don't mention it.

[rijk]

By the way, I think there is a bug in your New example:

[everweij]

Thanks. What I meant was to include a ToggleLayer component like this in the package. Should be doable, right?

Ah, now I understand. Well, to be honest, I'm a bit hesitant towards including deprecated / old stuff in v2.0.0. From your point of view I understand that including a kind of wrapper to make it backwards compatible is preferred, but it has some downsides:

• I'll have to 'support' it
• It contributes to a larger bundle size

Maybe it is wise to first try to migrate to v2, I'd be happy to help with that.

[everweij]

I've also updated the readme a bit to better explain the intention behind preferX / preferY -> in the options-table.

Well, I still don't understand..

Reading all this, I still expect the layer to be at the bottom of the trigger, not the top 🤔

I agree this is the most confusing part of all available options! Here's an example of exactly where the option is for:

It has impact on the opposite direction / axis of the preferred placement. It's kinda like flex-box's align-items / align-self. So in this example:

• I've selected the preferred placement 'top-center'
• this means that the placement priority flows from top to bottom
• half-way are the 'left-' and 'right=' placements
• the preferX prop helps to decide which side gets priority over the other one, given that both sides will fit

[everweij]

Has scrollOffset been removed by the way? The release notes don't mention it.

Thanks for noticing! Well, the option has been renamed: scrollOffset > containerOffset. I will fix this in the release notes right away!

[rijk]

I'll close this one, will reopen if I run into the issue again but assume it's fixed. Thanks for the help so far.