-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
@mtrutledge Any oppinion on this workaround? Have a better idea on this or my solution makes sense for now? |
@valadas can you take a look at the conflict? |
Oh, it is the minified result of the build, that will be re-generated by CI, should not even be tracked in my opinion... I think I can just remove it, hold on, I will try |
Hmm, I deleted the file and pushed but it still sees that as a difference, not sure how to correct that. @ohine any advice? or you just cherry-pick it? |
That's weird your delete didn't fix it. Sure thing, I nuke it and merge shortly. Thanks for giving it a try! |
Yeah, if it existed before the PR and I delete it, it is still seen as a change, Ideally I should have just not included in the the commit, but not sure how to undo that. |
I think we should delete the bundles on all those modules and add those filenames in the gitignore file. It will prevent those merge conflicts and the CI builds them anyway. Do you agree @mtrutledge ? |
Please do not merge yet. Looks like that bug got fixed in their latest release. I will give it a try and submit another PR if it works... |
Instead of deleting the file, try reverting all changes to that file in your PR @valadas, that way it will no longer cause any conflicts. And then create a new PR to remove the file. |
Hmm, strange I commented this yesterday but it did not save. Please do not merge this, the issue might have been fixed in their latest release and this workaround not needed. Testing now, will closes if tests are conclusive. |
Replaced by #184 I tested their new release and it works fine now, so the new solution is to upgrade instead of limit to an older version |
The latest react-portal-tooltip version has an issue with the latest React version. This PR limits it to the latest compatible verion. Fixes errors with tooltips and allows the Security module (and probably other modules) to not fail because of this bug.
closes #182