feat(Popup): Provide option to handle vertical offset #2444
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2444 +/- ##
==========================================
+ Coverage 99.74% 99.74% +<.01%
==========================================
Files 154 154
Lines 2699 2700 +1
==========================================
+ Hits 2692 2693 +1
Misses 7 7
Continue to review full report at Codecov.
|
|
Whoops, appears we also have #2450, where I've left some updated comments. I'll let you two decide which PR to keep around. If you should choose to update your PR, please see the comments I left there. |
bharatzvm
force-pushed
the
changes-bp
branch
2 times, most recently
from
db09bbd
to
eb46e2b
Compare
|
@levithomason I have made some updates according to the discussions in #2450 and #1146, waiting for your opinion. Sorry for delay in response, I was travelling. |
src/modules/Popup/Popup.js
Outdated
|
|
||
| applyVeriticalOffset = ({ top, bottom }, offset) => { | ||
| if (_.isNumber(top)) return { bottom, top: top + offset } | ||
| return { top, bottom: bottom + offset } |
There was a problem hiding this comment.
@bharatzvm, theverticalOffset calculation should be different based on if the popup is located above or below the trigger. Are you sure this logic will not work as expected when the popup is below the trigger?
There was a problem hiding this comment.
@adam-26 I followed the notion of moving the popup outwards of the screen through the centre of the screen from the explicitly positioned direction whichever it is. It was also one of the reason why I had to add the offset.
There was a problem hiding this comment.
@adam-26 My bad again, I was wrong thanks for spotting it.
There was a problem hiding this comment.
@adam-26 I think the logic works fine irrespective of the positioning because if it can't place the popup in the view port it tries all other positions until it find ones in the viewport if it cant, it falls back to the position user specified position. I will be updating the pr to fix these reviews
src/modules/Popup/Popup.js
Outdated
| return style | ||
| applyHorizontalOffset = ({ right, left }, offset) => { | ||
| if (_.isNumber(right)) return { left, right: right + offset } | ||
| return { right, left: left + offset } |
There was a problem hiding this comment.
@bharatzvm, the original code subtracts the horizontalOffset value from the left/right value, you are adding the offset. Have you made a code change somewhere else that inverts the logic?
There was a problem hiding this comment.
@adam-26, Consider my previous comment here too(But my bad, I didn't realise this change while creating the pr)
Completely inspired from Semantic-Org#1146 and its comments
Updated Popup-tests
Updated Examples usage examples
Updated Popup typing
|
@bharatzvm PR #2450 has been updated and merged. I will close this one. Thank you for working on this and for collaborating with @adam-26. |
Breaking Change
Popup now supports vertical offset alongside horizontal offset, in process offset prop
had to be renamed explicitly to mention the axis.
Before
After