-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🪟 🎉 Add datepicker for date/date-time fields in connector form #19678
Conversation
airbyte-webapp/src/views/Connector/ConnectorForm/components/Property/PropertyLabel.tsx
Outdated
Show resolved
Hide resolved
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.
A couple small comments and one visual bug I found when testing
airbyte-webapp/src/components/ui/DatePicker/DatePicker.module.scss
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/views/Connector/ConnectorForm/components/Property/Control.tsx
Show resolved
Hide resolved
…scss Co-authored-by: Lake Mossman <lake@airbyte.io>
@flash1293 looks like you're using the Bing Ads source connector, right? That connector spec unfortunately doesn't specify a "reports_start_date": {
"type": "string",
"order": 5,
"title": "Reports replication start date",
"format": "date",
"default": "2020-01-01",
"description": "The start date from which to begin replicating report data. Any data generated before this date will not be replicated in reports. This is a UTC date in YYYY-MM-DD format."
} Which is what we depend on to do regex validation here when generating the yup schema. So any old string can be passed in 😕 Unfortunately quite a few connector specs are missing |
Should we auto-add this pattern if it doesn't exist or will it break things? If it's too dangerous to do it, we should probably extend the SAT to cover this situation (out of scope for this PR I'd guess) |
I'm not sure I have enough context on the date types (there are five) to know if this would break anything - probably not? But my feeling is that keeping the connector definition specification as the source of truth for validation is the best approach. I'd rather advocate for consistency in the connector specs than patching missing fields on the frontend. |
+1 to the timeCaption suggestion Yeah I think the problem with trying to enforce the regex pattern in the frontend if it is missing on the connector spec is that different connectors may require different timestamp formats, so we can't implement a blanket logic to cover all connectors correctly. Though @sherifnada or @girarda correct me if I'm wrong here - I assume some timestamp formats are API-specific, but if it actually can be made consistent across all APIs that would be great |
+1 for that, I think going with the current logic for now and extending the SAT is a good approach to get to a clean state without breaking things in the meantime. |
@flash1293 thanks for the idea about adding |
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.
Changes LGTM, tested locally once more and it seems to be working as expected. Nice work!
agreed. But this will take a little bit longer than we want to merge this PR :) so I think we shouldn't assume the regex on the frontend |
What
date
ordate-time
is rendereddatepicker.mov
How
react-datepicker
and styles it with our design tokens<Datepicker />
component which renders a text input with a datepicker button.format
ofdate
ordate-time
will see the datepicker. Many connectors today have date fields that are typed asstring
. In these cases a normal text input will be rendered (see "End date" in the video above).Recommended reading order
Top to bottom
🚨 User Impact 🚨
The datepicker is currently behind the
connector.form.useDatepicker
experiment flag. It's enabled by default on OSS, and can be toggled easily on LaunchDarkly for cloud.The plan is to roll out the datepicker to all users on cloud as well, and remove the flag if all goes well. Even if there is a bug with the datepicker itself, a normal
<Input />
is rendered where the user can enter the UTC timestamp manually.