Skip to content

Conversation

@stanlp1
Copy link
Contributor

@stanlp1 stanlp1 commented Apr 25, 2024

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

  • I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
  • I have added JSDoc comments to my TypeScript definitions for improved documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added any necessary documentation (if appropriate).
  • I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Documentation content changes
  • TypeScript type definitions update
  • Other... Please describe:

@stanlp1 stanlp1 requested a review from a team April 25, 2024 22:05
@linear
Copy link

linear bot commented Apr 25, 2024

CI-3307 [MVP - OS UI PLP] - Add tests for useProductCard hook

Business Outcome:

  • We want good test coverage to be confident in our code

Definition of done:

  • Client-side test
  • Server side test
  • Take inspiration from already implemented tests for other hooks.

}

const getPrice = state?.itemFieldGetters?.getPrice;
const getPrice = tryCatchify(state?.itemFieldGetters?.getPrice);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I don't know what we should be doing here other than try catching it.

I was thinking of doing this to everything in itemFieldGetters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cspeller still complaining. Lets add the word Catchify to the list of cspeller words

src/utils.tsx Outdated
try {
return func(...args);
} catch (e) {
// do nothing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add something like the logger and have call it here? It would only console log in dev mode. That way we can let the user know there is a problem with the implementation during development but don't break anything in prod if the value is missing

@stanlp1 stanlp1 requested a review from esezen May 2, 2024 05:35
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@stanlp1 stanlp1 merged commit 8dfd620 into main May 6, 2024
@stanlp1 stanlp1 deleted the ci-3307-mvp-os-ui-plp-add-tests-for-useproductcard-hook branch May 6, 2024 19:57
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.

4 participants