open-amdocs / webrix

Powerful building blocks for React-based web applications
https://webrix.amdocs.com
Apache License 2.0
431 stars 31 forks source link

Poppable: Placement of {top:0, left:0} does not work #75

Open DieTapete opened 2 years ago

DieTapete commented 2 years ago

Prerequisites

Demo Page

https://codepen.io/dietapete/pen/OJxGgVQ

Explanation

ykadosh commented 2 years ago

Hi @DieTapete

There are a few issues with your example:

  1. You're not passing a reference to <Poppable/>, which is why you're modal is hidden. You should pass it like so:
    const App = () => {
    const ref = useRef();
    return (
    <div>
      <button ref={ref}/>
      <Poppable reference={ref}/>
    </div>
    );
    }
  2. You're getPlacements function returns [{top: 0, left: 0}]. You should calculated those values based on the rbr (reference bounding rect, i.e. the button) and tbr (target bounding rect, i.e your modal):
    const getPlacements = (rbr, tbr) => [
    {top: rbr.top - 100, left: rbr.left}
    ];
ykadosh commented 2 years ago

I see now that you wanted it to appear at [0,0], and indeed it considers it to be hidden in that case. @yairEO we use [0,0] as a hidden placement

see:

@DieTapete meanwhile you can use [1,1] until we fix this...

DieTapete commented 2 years ago

thanks for taking care of this. i'm using [1,0] for the time being ;)

yairEO commented 2 years ago

@ykadosh - what is a "hidden placement" purpose? why 0, 0 is so? I'm unsure how to address this bug because I don't know the reason behind this logic:

export const HIDDEN_PLACEMENT = {top: 0, left: 0, name: 'hidden'};
ykadosh commented 2 years ago

@yairEO I don't recall all the original reasoning, but the HIDDEN_PLACEMENT is used both for hiding the <Poppable/> (initially or when the target is missing), and as a communication device between the different "recovery strategies".

So there are a few recovery strategies, i.e. trap(), reposition() and hide(), and we have some logic to use them based on their output (see: https://github.com/open-amdocs/webrix/blob/master/src/components/Poppable/strategies/index.js#L33)

Since their output is an object with top and left, we used a special object to indicate the special case of when the <Poppable/> is hidden. We need to find a better way to communicate between those functions, but keep in mind that the final output must be a positioning object (with top and left).

DieTapete commented 2 years ago

Maybe it's not the "right" solution but would it be enough to set the hidden placement to something like Infinity?

ykadosh commented 2 years ago

The placement object must be something that can be used in CSS to position the popup, so Infinity cannot work unfortunately.