feat: Fetching index suggestions and dealing with the states CLOUDP-311786 #6887
Conversation
package.json
Outdated
| @@ -103,5 +103,9 @@ | |||
| "chai-enzyme": { | |||
| "cheerio": "1.0.0-rc.10" | |||
| } | |||
| }, | |||
| "dependencies": { | |||
| "@leafygreen-ui/skeleton-loader": "^2.0.11", | |||
There was a problem hiding this comment.
Can we add this dependency to the packages/compass-components/package.json instead?
Similar with the mongodb-mql-engines dependency, let's have that in the compass-indexes package.json
| marginBottom: spacing[600], | ||
| padding: spacing[600], | ||
| background: palette.gray.light3, | ||
| border: `1px solid ${palette.gray.light2}`, |
There was a problem hiding this comment.
We add a dark mode styles so this border looks good in both themes.
There was a problem hiding this comment.
https://jira.mongodb.org/browse/CLOUDP-313986 dark mode ticket is here! will go through all the components at that point and make sure it looks good in dark mode
| @@ -117,7 +154,7 @@ const QueryFlowSection = ({ | |||
| <div className={editorActionContainerStyles}> | |||
| <Button | |||
| onClick={() => { | |||
| // TODO in CLOUDP-311786 | |||
| void handleSuggestedIndexButtonClick(); | |||
There was a problem hiding this comment.
nit: Could we instead pass the handleSuggestedIndexButtonClick here, and then in the callback method there use void? That way we're passing a constant function here most of the time.
| fetchingSuggestionsState: IndexSuggestionState; | ||
|
|
||
| // error specific to fetching index suggestions | ||
| fetchingSuggestionsError: Error | string | null; |
There was a problem hiding this comment.
Looking at the code we could make this type just string | null. That being said I think we'd ideally want to be using the Error type here, so in those places where we set this error we can do
fetchingSuggestionsError: new Error('ERROR_STRING')and then update this type to be only Error | null.
There was a problem hiding this comment.
how come you recommend Error instead of string? i'm basing it off of ErrorEncountered and that one seems to be string
| sampleDocs: sampleDocuments, | ||
| indexSuggestions: null, | ||
| fetchingSuggestionsError: | ||
| 'Error parsing query. Please follow query structure.', |
There was a problem hiding this comment.
Should we include anything from the error here? I'm wondering if the message might help folks.
There was a problem hiding this comment.
fixed it but note that it will not show up right now, going to fix up the error state and disabled in another ticket but just added some of it in this one
| sampleDocs: sampleDocuments, | ||
| indexSuggestions, | ||
| fetchingSuggestionsError: | ||
| 'No suggested index found. Please choose Start with an Index at the top to continue.', |
There was a problem hiding this comment.
Maybe:
| 'No suggested index found. Please choose Start with an Index at the top to continue.', | |
| 'No suggested index found. Please choose "Start with an Index" at the top to continue.', |
(fine as is too, feel free to resolve).
| ); | ||
|
|
||
| const query = mql.parseQuery( | ||
| EJSON.parse(inputQuery, { relaxed: false }), |
There was a problem hiding this comment.
Does this work as expected with some queries? On a quick glance we might need to be parsing them with a different method. Should it be using this instead?
I might be wrong, haven't checked out the branch and tried it yet.
There was a problem hiding this comment.
it works for the most part but will break if there's a new line
working:
breaks:
Description
https://jira.mongodb.org/browse/CLOUDP-311786
With data:
Loading:
Error state to be fully implemented in another ticket
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes