Remove object-assign-deep, refactor setting popper options#516
Conversation
|
@genadis removing would be great! Can you add some tests to ensure the deeply nested objects are merged? |
|
@genadis I can add the tests real quick, never mind, but in the future please add tests for all new PRs 😃 |
|
@genadis I made some changes and added tests. Any issues with the changes? If not, I will merge. |
RobbieTheWagner
changed the title
|
Went ahead and merged with my changes. If you have any issues. Please let me know! |
|
@rwwagner90 I believe your changes have a number of issues, I've added comments inline |
|
this: #486 was the original issue |
|
@genadis what are the issues? I do not see any comments and the issue you referenced where the shepherd class is not added is no longer an issue. |
|
@genadis I see the issue now, but I don't see your inline comments. Will work toward a fix. We probably should have kept deep assign. |
|
@genadis I just pushed the fix to master. Please let me know if you are still seeing issues or have suggestions. |
| }; | ||
|
|
||
| const tippyOptions = makeAttachedTippyOptions(attachToOpts, stepOptions); | ||
| expect(tippyOptions.popperOptions.modifiers.foo).toBe('bar'); |
There was a problem hiding this comment.
The issue was that if you have custom modifiers, shepherd poper modifiers get removed.
The tests does not check it
|
|
||
| popperOptions = { | ||
| ...popperOptions, | ||
| ...tippyOptions.popperOptions |
There was a problem hiding this comment.
if tippyOptions.popperOptions has modifiers they will override the tippy modifiers, which was the issue in the first place.
please look at my original Pull request that solves the issue.
| if (step.options.tippyOptions && step.options.tippyOptions.popperOptions) { | ||
| popperOptions = { | ||
| ...popperOptions, | ||
| ...step.options.tippyOptions.popperOptions |
There was a problem hiding this comment.
if step.options.tippyOptions.popperOptions has modifiers they will override the tippy modifiers, which was the issue in the first place.
please look at my original Pull request that solves the issue.
yes, sorry, didn't submit it. now submitted |
looks good, thanks |
2 participants
Following #488
using 1K object-assign-deep seems overkill.
especially since their readme starts with: "Caution! Danger of Death!"
It introduces unnecessary risk fo user error.