Skip to content

Rewrite react-vega #613

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
Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Rewrite react-vega #613

wants to merge 12 commits into from

Conversation

JakeAdler
Copy link
Contributor

@JakeAdler JakeAdler commented Jul 9, 2025 •
edited
Loading

#612

TODO

  • Add test workflow
  • Commitlint

It's been a while since I've published a npm package so it's very possible I'm missing something as it relates to that process. Ty in advanced for any feedback you may have :) cc @domoritz @kristw

@JakeAdler JakeAdler changed the title rewrite react-vega [WIP] rewrite react-vega Jul 9, 2025
@JakeAdler JakeAdler marked this pull request as ready for review July 13, 2025 02:22
@JakeAdler JakeAdler changed the title [WIP] rewrite react-vega Rewrite react-vega Jul 16, 2025
domoritz
domoritz approved these changes Jul 18, 2025
View reviewed changes
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Awesome. Just a few comments but I think we are mostly good to merge and release.

@domoritz
Copy link
Member

Let me know when this is ready for a review.

@JakeAdler JakeAdler requested a review from domoritz August 3, 2025 01:44
@JakeAdler
Copy link
Contributor Author

Let me know when this is ready for a review.

Ready for review 👍 @domoritz

@domoritz
Copy link
Member

domoritz commented Aug 6, 2025 •
edited
Loading

Please resolve the merge conflicts. Can we remove netlify then?

domoritz
domoritz approved these changes Aug 6, 2025
View reviewed changes
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! Just a few comments and then we can merge.

@JakeAdler
Copy link
Contributor Author

Will get around to addressing feedback this weekend, thanks for patience all!

@JakeAdler
Copy link
Contributor Author

FYI I added husky back, it's just the easiest way to set up a pre-commit hook that I could find. We now run tsc, eslint, and prettier for each commit. Other than the outstanding comments that I've replied to, all other feedback should be addressed. Thanks for the patience again everybody.

@JakeAdler JakeAdler requested a review from domoritz August 17, 2025 20:31
@domoritz
Copy link
Member

Can you resolve the merge conflicts? I am personally not a fan of precommit hooks. Let's remove them and instead add ci tests?

domoritz
domoritz approved these changes Aug 23, 2025
View reviewed changes
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

See my comment on ci linting

@JakeAdler
Copy link
Contributor Author

JakeAdler commented Aug 23, 2025 •
edited
Loading

Removed husky and the pre-commit hook, updated the license, added linting in CI, and resolved merge conflicts. Let me know if there is anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domoritz domoritz domoritz approved these changes

+3 more reviewers

@ties ties ties left review comments

@kristw kristw kristw left review comments

@yf-yang yf-yang yf-yang left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JakeAdler @domoritz @ties @kristw @yf-yang