-
Notifications
You must be signed in to change notification settings - Fork 305
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
feat(metadata-sidebar): Add handler for Autofill button #3700
Conversation
src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
Outdated
Show resolved
Hide resolved
src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
Outdated
Show resolved
Hide resolved
src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
Outdated
Show resolved
Hide resolved
0bed1bb
to
21d879a
Compare
src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
Outdated
Show resolved
Hide resolved
And bump metadata-editor package Wrap MetadataInstanceList and MetadataInstanceForm in Autofill provider Co-authored-by: Wiola (wpiesiak)
3f6575d
to
6243548
Compare
async (templateKey: string, fields: MetadataTemplateField[]) => { | ||
const aiAPI = api.getIntelligenceAPI(); | ||
|
||
await wait(1000); |
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.
Why do we have a setTimeout here? We should have a constant for the delay time as well.
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.
sorry, I was using it to test loading state, and forgot to delete
async (templateKey: string, fields: MetadataTemplateField[]) => { | ||
const aiAPI = api.getIntelligenceAPI(); | ||
|
||
await wait(1000); |
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 is the reason for this ?
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.
sorry, I was using it to test loading state, and forgot to delete
src/test-utils/testing-library.tsx
Outdated
<AutofillContextProvider | ||
isAiSuggestionsFeatureEnabled={isAiSuggestionsFeatureEnabled} | ||
fetchSuggestions={fetchSuggestions} | ||
isAiSuggestionsFeatureEnabled={isAiSuggestionsFeatureEnabled} | ||
> |
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.
after looking at this again and noticed we are passing props specifically for it, think we need to consider moving this out of the testing-library into the metadata-editor tests directly, all the tests in the repo will use this wrapper eventually, i cant imagine that provider being used that much to warrant being in the wrapper. the other providers where are more generic
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.
Agree. AutofillContextProvider
is needed only for MetadataInstanceEditor
tests.
I did tried moving it out but it resulted in failing tests
TypeError: (0 , _reactIntl.useIntl) is not a function
Not quite sure what's happening. But I needed to move the whole wrapper component to MetadataInstanceEditor
test.
@@ -75,6 +75,7 @@ class Intelligence extends Base { | |||
suggestionsResponse = await this.xhr.post({ | |||
url, | |||
data: request, | |||
id: `file_${request.items[0].id}`, |
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.
i see we do this in the ask
method as well but how important is this id
field? if items
has multiple files would this cause conflicts or unexpected behavior since we always look at the first index?
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.
id
is used internally by post to get auth token, it can by an array, but for this endpoint we only use one file. That is whybI take first element.
@@ -22,9 +24,8 @@ export interface MetadataInstanceEditorProps { | |||
template: MetadataTemplateInstance; | |||
} | |||
|
|||
const MetadataInstanceEditor: React.FC<MetadataInstanceEditorProps> = ({ | |||
export const MetadataInstanceEditor: React.FC<MetadataInstanceEditorProps> = ({ |
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.
nit: if we're exporting for unit tests, we usually add the export at bottom with something like:
export { MetadataInstanceEditor as MetadataInstanceEditorComponent };
example:
box-ui-elements/src/elements/content-sidebar/Sidebar.js
Lines 342 to 343 in b6b61bf
export { Sidebar as SidebarComponent }; | |
export default flow([withCurrentUser, withFeatureConsumer, withRouter])(Sidebar); |
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.
We actually don't need the withApiWrapper
so I will restore the old way we were exporting here.
async (templateKey: string, fields: MetadataTemplateField[]) => { | ||
const aiAPI = api.getIntelligenceAPI(); | ||
|
||
await wait(1000); |
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 reasoning for this? are we adding an artificial loading time?
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.
sorry, I was using it to test loading state, and forgot to delete
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.
comment but approved. please address in follow up if not fixed in this PR
src/test-utils/testing-library.tsx
Outdated
); | ||
|
||
type RenderConnectedOptions = Omit<RenderOptions, 'wrapper'> & { | ||
type RenderConnectedOptions = RenderOptions & { | ||
// Omit<RenderOptions, 'wrapper'> & { |
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.
uncomment or remove?
b17dc53
to
3d8ba70
Compare
No description provided.