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
#6269 Auto-configure Google Drive PKCE integrations #6734
Conversation
*/ | ||
initialRecord?: Row | ((x: Row) => boolean); | ||
forceShowRecord?: Row | ((x: Row) => boolean); |
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.
What's the purpose the change to initialRecord?
It looks like this change (and the change the useEffect dependency) changes the behavior of the prop. Instead defaulting the initial value, it now locks to that record even when the row data changes
We'll need to audit the call-sites to ensure that it's not breaking some intended behavior
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 intended behavior wasn't working to begin with, I was just trying to fix it
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.
@BLoe can you add a feature test for this to the release QA sheet? https://www.notion.so/pixiebrix/Release-1-8-2-676f3ca1795541019e25d79c306b04c9
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.
Sure, done
src/hooks/useSetDocumentTitle.ts
Outdated
|
||
/** | ||
* Set title of the document, restoring the original title when component is unmounted. | ||
*/ | ||
export function useTitle(title: string): void { | ||
export function useSetDocumentTitle(title: string, show?: boolean): void { |
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.
👍 Thanks for renaming the hook
src/extensionConsole/pages/integrations/IntegrationConfigEditorModal.tsx
Outdated
Show resolved
Hide resolved
💅 Thanks for improving the naming! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6734 +/- ##
==========================================
- Coverage 70.18% 70.05% -0.13%
==========================================
Files 1182 1188 +6
Lines 36825 36939 +114
Branches 6932 6937 +5
==========================================
+ Hits 25845 25878 +33
- Misses 10980 11061 +81
☔ View full report in Codecov by Sentry. |
When the PR is merged, the first loom link found on this PR will be posted to |
d## What does this PR do?
IntegrationConfigEditorModal
Reviewer Tips
IntegrationsPage
andIntegrationConfigEditorModal
Discussion
Remaining work
Pull out the auto-configure logic to use in AuthWidget as wellDemo
https://www.loom.com/share/0b3dec922cda447c959099b2a6442e69
Checklist
IntegrationsPage
could probably use a full test suite but that's a lot of work right now and blows up the scope of this ticket too muchsrc/tsconfig.strictNullChecks.json
(if possible)