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.

Sign up for GitHub

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

Jump to bottom

Add types: VictorySharedEvents #1520

Merged
merged 6 commits into from
Apr 23, 2020
Merged

Add types: VictorySharedEvents #1520

merged 6 commits into from
Apr 23, 2020

Conversation

kale-stew
Copy link
Contributor

This PR introduces types for VictorySharedEvents and updates our spelling of Labelable across 3 different interfaces.

Tested by running yarn nps check.dev locally

@boygirl
Copy link
Contributor

boygirl commented Apr 21, 2020

@kale-stew thank you for the spelling consistency! It looks like VictorySharedEvents types has a couple omissions, but this is looking great otherwise.

@boygirl
Copy link
Contributor

boygirl commented Apr 21, 2020

@kale-stew looks like this is failing linting. Mind running nps check.dev?

@kale-stew kale-stew force-pushed the types/victory-shared-events branch 2 times, most recently from 9f100ee to 5842e1d Compare April 22, 2020 22:08
@boygirl
Copy link
Contributor

boygirl commented Apr 22, 2020 •
edited

@kale-stew the build is failing on nps compile-ts with an error related to labels on VictoryCandlestick. Maybe your naming changes conflicted with something Wendy was working on that got merged. Mind taking a look?

@kale-stew
Copy link
Contributor Author

@boygirl oddly enough, that's all succeeding for me locally, and that's why I've been confused about reconciling the issue. I just rebased behind the latest in master, I think I've got things fixed now.

@kale-stew kale-stew force-pushed the types/victory-shared-events branch 2 times, most recently from 0bebf76 to f50f106 Compare April 23, 2020 18:45
boygirl
boygirl approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@boygirl boygirl left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the spelling consistency improvements too!

@kale-stew kale-stew merged commit 860aad9 into master Apr 23, 2020
@kale-stew kale-stew deleted the types/victory-shared-events branch April 23, 2020 19:37
@wsparsons wsparsons mentioned this pull request Apr 23, 2020
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boygirl boygirl boygirl approved these changes

@maddles maddles Awaiting requested review from maddles

@wsparsons wsparsons Awaiting requested review from wsparsons

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kale-stew @boygirl