search

SteffeyDev / react-native-popover-view

A well-tested, adaptable, lightweight <Popover> component for react-native
MIT License
614 stars 92 forks source link

computeAutoGeometry enhancement #96

Closed RajRohitYadav closed 3 years ago

RajRohitYadav commented 3 years ago

new optional prop added: priorityPlacement This prop is an array with a list of sides based on priority.

ex: priorityPlacement = {['left', 'bottom']} will try to render first left, then bottom. If failed it will find the best space to render it, based on available space on all sides.

computeAutoGeometry is equipped with handling priorityPlacement based on the best area available for the popover. Also, new logic in computeAutoGeometry will make sure popover is rendered more gracefully in best available area.

SteffeyDev commented 3 years ago

Thanks for the PR, I like the idea a lot! You're work on computeAutoGeometry is appreciated.

What do you think of, instead of creating a new prop, overloading the existing placement prop to accept an array in addition to a single value? That way, when they pass in an a single value, it would be treated as an array with one value, and then all placements would be evaluated as an array. I'm thinking something like this:

['top'] // Always place on top
['top', 'bottom'] // Place on top if it fits, otherwise on bottom.  If it doesn't fit top or bottom, prefer top (first in list)
['top', 'bottom', 'auto'] // Try top, then bottom, then fallback to auto placement (pick best fit)
['right', 'bottom', 'top'] // You get the idea...
['auto'] // Use auto placement (default)

What are your thoughts? I'm not necessarily asking you to make this change, just want to discuss for now.

RajRohitYadav commented 3 years ago

Yup, sounds good. Implementing this in existing props is more sensible. However, should take care of backward compatibility.

So, I will remove the placementPriority prop and logic for now. Only auto-placement enhancement logic will be there.

I will make another PR for the suggested change of using placement for handling an array and string.

RajRohitYadav commented 3 years ago

I have done the changes. Removed placementPriority and related code.

SteffeyDev commented 3 years ago

Ok, sounds good!

RajRohitYadav commented 3 years ago

Hey @SteffeyDev,

I was planning to start the suggested change today, but after a pull from base, there are errors. After clearing those errors in my local I found the popover is not working as expected.

Please have a look.

SteffeyDev commented 3 years ago

Sorry about that, I was doing some refactoring and didn't fully test it. I'll get that fixed.

SteffeyDev commented 3 years ago

I just tested and everything is working as expected, @RajRohitYadav can you be more specific about the errors you are seeing?

SteffeyDev commented 3 years ago

@RajRohitYadav I discovered the errors and have fixed them. Also, updated the repo so that all development is done on the develop branch, so please switch to using that branch for PRs.

RajRohitYadav commented 3 years ago

Thank you for the update, I will work on it asap

RajRohitYadav commented 2 years ago

Hello @SteffeyDev, have made PR for the above changes in #141 I have tested it, but need your help in testing as I'm not aware of the edge/other cases in the product.