-
Notifications
You must be signed in to change notification settings - Fork 251
Onboarding v3 #1881
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
base: main
Are you sure you want to change the base?
Onboarding v3 #1881
Conversation
| const { parameters, description, prompt, rowCount, name, fromCloud } = | ||
| parsedInput | ||
|
|
||
| if (fromCloud && !env.LATITUDE_CLOUD) { |
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 fromCloud is needed? we should not trust the client
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 agree, but just making it more comfortable for them. If they want to say false, it will break for them when creating the dataset
| return ( | ||
| <div className='flex flex-row items-center gap-10 h-full w-full'> | ||
| <div className='flex flex-col items-end w-full h-full'> | ||
| <div className='relative flex-1 w-full max-h-[350px] max-w-[600px]'> |
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.
Adding max-w-[600px] is repeated in all sub components. Is an smell that should be in the parent
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've changed it, adding a max to the root, but I had to add a min width now because we want the editor to be 560px and when it wasnt there, it would distribute equally in both columns taking away space from the editor
| } satisfies BlockRootNode | ||
| } | ||
|
|
||
| export const emptyRootBlock: BlockRootNode = { |
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.
Constants are all UPPER_CASE
| currentWorker.terminate() | ||
| setWorker(null) | ||
| } | ||
| reset() |
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 used to not reset the worker after we closed it, making if we logged in with another account, the prompt from the previous user to be in the new users editor
This is the whole PR to add the new onboarding in the application, under a feature flag. We were going to divide it into smaller PRs, but the whole thing is small enough to review well.