-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make v2 default #50
Make v2 default #50
Conversation
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.
Functionally works , tested locally
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.
all for having a default to v2 👍
Do we have any agreed formatting rules for this repo? 😆
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.
Two tiny comments but other than that, looks good 👍
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.
Re-approved following the new changes 👍
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.
The formatting changes make this hard to review. It should have been separate. Plus formatting rules aren't enforced at the moment, so some else's IDE might be formatting differently. Happy for the formatting changes but it would have been nice if it was in a different PR.
Yeah you're right. I'm happy to split them out into another PR. Ill do that 👍 |
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 !
What is the context of this PR?
This is to change the version dropdown to default to v2. Just to make things a bit easier for the user.
ref_p_start_date
andref_p_end_date
not being usedHow to review