-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: remove lodash dependency #6809
base: main
Are you sure you want to change the base?
refactor: remove lodash dependency #6809
Conversation
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6809 +/- ##
==========================================
+ Coverage 81.49% 81.52% +0.03%
==========================================
Files 399 402 +3
Lines 12978 13028 +50
Branches 4270 4287 +17
==========================================
+ Hits 10576 10621 +45
- Misses 2402 2407 +5
|
}; | ||
|
||
useEffect(() => { | ||
const observer = new IntersectionObserver( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some refactoring here to use IntersectionObserver
rather than a scroll event for tracking when the gradients should become visible. This should be more performant, also the behavior with the component using the throttle utility seemed subtly different than when using throttle
from lodash
so I opted to use an intersection observer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good — I do see some of the new utils aren’t covered by tests yet. Also might be some opportunity to clean up an extra clamp
util in Coachmark helpers.
Still have scroll gradient changes left to review though. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work. 🎉
Some small comments has added.
Build/type issues will be resolved when #6861 is merged |
Closes #6805
To help reduce our footprint, this PR removes
lodash
as a dependency, as everything we used fromlodash
could be done natively. I added utilities for everything we previously imported fromlodash
.Utilities replaced:
clamp
debounce
throttle
isEqual
I also added support for prefixing in our current
uuidv4
utility as this was possibly withuniqueId
fromlodash
(previously used inuseDatagrid
).What did you change?
Any file importing anything from
lodash
, now using our own utilities that do the same thingHow did you test and verify your work?
Manually verified in storybook and made sure all tests pass