-
-
Notifications
You must be signed in to change notification settings - Fork 108
Code clean-up #198
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
Code clean-up #198
Conversation
In this PR we; - Add more test - KISS more (no human lips were affected or damaged in this process) - file restructure - Add more types - clean up useRef usage -
@opmat @mosoakinyemi can you please give me a review here |
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.
LGTM! 👍
Left a few nit comments, but I'll approve so I don't block the PR. 🚀
__tests__/utils.test.ts
Outdated
buildKeyValueString, | ||
dynamicSplitObjectIsValid, | ||
paystackHtmlContent, | ||
} from '../development//utils/helper'; |
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.
[low]: Small typo in the path /development//utils/helper
|
||
result = buildKeyValueString('key2', undefined); | ||
expect(result).toBe(''); | ||
}); |
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.
Nice!
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.
thanks :)
firstName, | ||
amount = "0.00", | ||
currency = "NGN", | ||
channels = ["card"], |
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.
[nit]: let's declare these initial values as consts, just like CLOSE_URL
so it looks like:
amount = DEFAULT_AMOUNT
currency = DEFAULT_CURRENCY
channels = DEFAULT_CHANNELS
export interface PayStackRef { | ||
startTransaction: () => void; | ||
endTransaction: () => void; | ||
} |
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.
[Suggestion]: It’d be great to add JSDoc comments to the props in the type definitions. This would show helpful hints to library users during development and make the API easier to use and understand at a glance. (This could be done later in a follow-up PR or sth)
Example:
export interface PayStackRef {
/**
* Triggered when a transaction starts.
*/
startTransaction: () => void;
/**
* Triggered when a transaction ends.
*/
endTransaction: () => void;
}
development/utils/index.ts
Outdated
? `metadata: ${metadata},` | ||
: `metadata: { custom_fields: [{ display_name: '${firstName + " " + lastName}', variable_name: '${billingName}', value:'' }]},`; | ||
}; | ||
|
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.
[nit]: Great utility functions! Could you move them to a utils/strings.ts
file and then re-export them from utils/index.ts
? That way, they can be imported cleanly like this:
import {buildKeyValueString} from '../utils'
🎉 This PR is included in version 4.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In this PR we;