-
Notifications
You must be signed in to change notification settings - Fork 3
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: show wizard data on Editor UI from BOS #936
feat: show wizard data on Editor UI from BOS #936
Conversation
…-wizard-data-to-generate-sample-code-and-schema
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.
Mostly good. Some comments on future plans and code clarity.
if (!response.ok) { | ||
throw new Error('Network response was not ok'); | ||
} | ||
const data = await response.json(); |
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.
Can we validate the data returned as well? To make sure it has jsCode and sqlCode fields? And how do we handle a failure here visually? If something goes wrong, what would be the next best customer experience? Being redirected to a page which lets them explore existing indexers? The Create new indexer page?
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.
Yeah I can validate it to check for a correct response. Im not sure what the correct user response would be if the generateAPI fails. we have already forcefully already navigated them to the editor component outside of BOS.
const childKey = `${item.method_name}::${property}`; | ||
if (checkboxState[childKey]) { | ||
acc[property] = item.schema.properties[property]; | ||
if (!item.schema) return null; |
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.
If you have an if statement like this, you don't need to have item.schema checks in following if statements. You can leave it out as you know following code is guaranteed to have item.schema. The same follows for any other things you check. If you want that to be readily apparent, you can leave in the else block that's omitted here. All subsequent code lives in the else instead. This leads to a lot of tabbing, which can then be a signal that maybe a refactor is needed. You can maybe try something like the following:
- Perform checks which return null
- Store combined conditionals as boolean with a name. E.g.
parentChecked || Object.keys(filteredProperties).length > 0
gets stored as a variable called hasPropertiesFromParent or something. As is, I have no clue what that if statement checks. - Move filter logic to its own function called
filterSchemaProperties()
- Move some conditional logic to return such as
...(var && { var })
which only spreads if var exists.
Something like that. In any case, my main point is this code is confusing. Can you refactor this code to make its logic more apparent at first glance?
When users start on launchpad/wizard and populate the form with a valid checkbox state, BOS will send a request that the nextjs frontend consumes to check if the current Editor UI is one with launchpad data. If this is the case, the editor will call the generate/api and display the wizard code on editor.