Invalid prop `offsetX` of type `string` supplied to `Popup`, expected `number`

[svobom57]

Hello, I'm not sure if this is a bug or a feature request. We have a use case where we need to offset the Popup using rems rather than px for the site to remain responsive. It works fine but it raises a warning.

Version

1.3.1

Test Case

https://codesandbox.io/s/reactjs-popup-issue-template-7b42d

Expected Behavior

When I set offsetX as a String value containing rems it shouldn't raise a warning.

Actual Behavior

It raises a warning

[zakariaelas]

It works fine

I might be wrong, but I dont think this works as one would expect. If you provide anything other than a number to offsetX (or offsetY), the coordinate will eventually resolve to NaN (code). From your code sandbox, if you try using any other value like "2rem" or "3rem", you will notice that the popup actually does not change its position relative to the x axis.

@yjose any thoughts on this please ?

[yjose]

Hi @svobom57 , for now, offsetX and offsetY only accept a number code.

@zakariaelas if you are interested to send a PR to support rems, you can use calc() CSS operator instead of simple javascript + / - operations. That's mean you need to change

  top = args[0] === 'top' ? top - offsetY : top + offsetY;

to

  top = args[0] === 'top' ? `calc(${top}px - offsetY)` : `calc(${top}px + offsetY)`;

and the same for left

also, need to check if offsetX is a number if it's the case you need to convert it to string with px in the end to make calc() will work correctly

[zakariaelas]

Alright, thanks. Will work on it.

[zakariaelas]

@yjose I tried looking into what you suggested a little further. Doing as you said will actually break checking if the calculated position is going out of bounds (Step 4 of the algorithm to calculate positions). The following check will not work as expected as strings will be compared to numbers:

if (
  contentBox.top <= wrapperBox.top ||
  contentBox.left <= wrapperBox.left ||
  contentBox.top + contentBox.height >=
    wrapperBox.top + wrapperBox.height ||
  contentBox.left + contentBox.width >= wrapperBox.left + wrapperBox.width
) {
  i++;
} else {
  break;
}

Proposition 1

If your intention is to support rems and only rems for now, we can make use of a computePxFromRems(remValue) that would take the offsetX as input from the user (if it is a rem value) and return the value as a number (representing the number of pixels). This would work fine and the library would support rems, however, you can see that the limitation is that, in the future, if we want to support other units like vw, vh, ems, etc.. we would have to write a function for each of those to handle the conversion to a number.

Proposition 2

We can pass the ContentEl ref to the calculatePosition function, and make use of the window.getComputedStyle() function on ContentEl to resolve the value of the calc() function. Following is a rough draft of the getCoordinatesForPosition function:

top = args[0] === 'top' ? `calc(${top}px - offsetY)` : `calc(${top}px + offsetY)`;
this.ContentEl.style.top = top;
top = parseInt(window.getComputedStyle(ContentEl).top);

Setting the ContentEl's top property in this case is primarily done to get the computed value. It will be overwritten once we exit from the calculatePosition function (code). This solution will make the library support different units used for offsetX and offsetY instead of just a number or rem. However it does indeed feel a little bit "awkward" implementation wise. The reason I feel that way is because I don't really like the idea a of passing the ContentEl ref to calculatePosition, and having to set its top and left style property at that stage. It's not really the responsibility of the function, and this would also make it less reusable in the future. However, one could argue that in our context, we are almost always calculating the position of "something". In our case, this "something" is an element we are eventually going to append to the DOM.

What do you think ? A better idea as to how to handle this ?