Skip to content
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

fix(zoombar): use Tools.isEmpty instead of lodash.isEmpty import #874

Closed
wants to merge 1 commit into from
Closed

fix(zoombar): use Tools.isEmpty instead of lodash.isEmpty import #874

wants to merge 1 commit into from

Conversation

moczolaszlo
Copy link

@moczolaszlo moczolaszlo commented Nov 9, 2020

Hi, our Webpack build throws an error with the latest version(s) of charts/react-charts packages:

ERROR in ./node_modules/@carbon/charts/components/axes/zoom-bar.js
 Module not found: Error: Can't resolve 'lodash/isEmpty' in '/node_modules/@carbon/charts/components/axes'

Updates

I changed the lodash.isEmpty import to the lodash-es version and refactored the code to use it from the Tools namespace, like Tools.isEmpty.
This could fix our build, too. 😄

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

@moczolaszlo moczolaszlo requested review from cal-smith and removed request for a team November 9, 2020 19:36
@theiliad
Copy link
Member

theiliad commented Nov 9, 2020

Hi, our Webpack build throws an error with the latest version(s) of charts/react-charts packages:

ERROR in ./node_modules/@carbon/charts/components/axes/zoom-bar.js
 Module not found: Error: Can't resolve 'lodash/isEmpty' in '/node_modules/@carbon/charts/components/axes'

Updates

I changed the lodash.isEmpty import to the lodash-es version and refactored the code to use it from the Tools namespace, like Tools.isEmpty.
This could fix our build, too. 😄

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

Hi, yes I've caught this error as well. Seems to have come into the codebase with a PR that we merged. I'll fix that

@moczolaszlo
Copy link
Author

Hi, yes I've caught this error as well. Seems to have come into the codebase with a PR that we merged. I'll fix that

Yes, this was that PR: #855

@theiliad
Copy link
Member

theiliad commented Nov 9, 2020

Hi, yes I've caught this error as well. Seems to have come into the codebase with a PR that we merged. I'll fix that

Yes, this was that PR: #855

Yes, we use lodash-es however the PR added an import to lodash. PR coming

@moczolaszlo
Copy link
Author

I'm not really understand what was the problem with this PR, because you made the same - that is more work (make a workbranch, PR, etc) 😄 . But the most important is the fix is released, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants