Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

feat: refactor arrow to typescript #175 #184

Merged
merged 6 commits into from
Mar 24, 2022
Merged

Conversation

cboyce183
Copy link
Contributor

Arrow component, with added types

Copy link
Member

@diondiondion diondiondion left a comment

Choose a reason for hiding this comment

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

Hi Charlie, how are things? :-)

Thanks for making a start working on this. I think it'd be nice if we could phase out PropTypes as we introduce TypeScript. And should the getArrowPosition function be converted to TS, too? (I haven't looked at it just now, just wondering if that would make things more consistent or robust)

src/Arrow/index.tsx Outdated Show resolved Hide resolved
src/Arrow/index.tsx Outdated Show resolved Hide resolved
@@ -37,7 +38,7 @@ function useArrowStyles(primaryPlacement, arrowSize) {
return arrowStyles;
}

const Arrow = forwardRef((props, ref) => {
const Arrow = forwardRef((props: {placement: string, size: number, distanceFromEdge: number, style}, ref: React.RefObject<HTMLElement>) => {
Copy link
Member

Choose a reason for hiding this comment

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

The placement prop can be defined more granularly based on the PLACEMENTS constant – or maybe you can even import that type from popperJS. Should result in a nice auto-complete for this prop, as only a limited number of strings are actually allowed as values.

Copy link
Member

@diondiondion diondiondion left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Charlie. Just some comments.

src/Arrow/index.tsx Outdated Show resolved Hide resolved

import {POPPER_PLACEMENTS} from '../constants';
Copy link
Member

Choose a reason for hiding this comment

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

It seems like you could still create a type from this constant as described here: https://stackoverflow.com/a/62900613

(At least if it's safe to convert the constants file to TypeScript, too.)

@cboyce183 cboyce183 merged commit a6b8de4 into master Mar 24, 2022
@cboyce183 cboyce183 deleted the ts-refactor-arrow branch March 24, 2022 12:10
@5app-Machine
Copy link
Contributor

🎉 This PR is included in version 14.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants