-
Notifications
You must be signed in to change notification settings - Fork 11
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
Core/Google Drive datasource #135
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.
@janaka This is ready for a look, left a comment on opendal
reader
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.
Left some comments essentially around how the datasource settings are implemented in a way that couples the backend to the UI too much.
If it makes it simpler drop the fancy folder selection UI for not (even though a nicer UX). But it feels like the creds setting still requires a new funcitonality unless we chang that to "paste your key here" which I don't think we should do.
Update poe env
0dc764b
to
aae68c6
Compare
* chore: Fix shared ask space selector not syncing with rest of the page. * chore: Make space selector expanded by default.
Co-authored-by: Janaka Abeywardhana <[email protected]>
Scanning through on my phone seems all good. Will look on my laptop in a bit. |
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 - Nice work cleaning it up.
Can you please make sure to do a full QA. That personal ask issue is the only known issue iirc.
Also, the integration tests broke for some reason a few PRs back, I couldn't figure it out so left it. Worth checking that as well.
Okay, Doing a full QA |
Description
Please include a summary of the change, e.g. what the new feature # is and/or what bug # it fixes. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes/Implements #(issue/feature)
implements #134
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration by modifying the list below.
Test Configuration:
Please describe the test setup. List them below as bullet points.
Checklist: