Setup Wizard: add new setup steps UI#47384
Conversation
5671a77 to
95efd97
Compare
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits c0fe380 and a4799cb or learn more. Open explanation
|
valerybugakov
left a comment
There was a problem hiding this comment.
Tested locally ✅
Most of the comments are related to the ongoing react-router v6 migration. More context can be found here.
client/web/src/SourcegraphWebApp.tsx
Outdated
| authenticatedUser === undefined || | ||
| graphqlClient === undefined || | ||
| temporarySettingsStorage === undefined || | ||
| !settingsCascade |
There was a problem hiding this comment.
Why do we block rendering because of the missing settingsCascade value?
There was a problem hiding this comment.
In the setup wizard we're using flag to check is the wizard available or not. If we load settings cascasde in async way we would see a flash between states, like if flag is true by default then we show wizard UI and then we load settings and in settings setup wizard is not available and we redirect you to the repositories page. As a result you can see wizard UI while we're loading setting cascade (same problem if it's false by default)
So I actually don't understand how other functionality which is behind feature flag in settings works at the moment (does it also have flashes between states?) all this time I've been thinking that we block rendering until we get settingsCascade and this is how it works but apparently it's not the case. So I added this check here. If you think we should not blocking the main UI let me know
client/web/src/setup-wizard/components/setup-steps/SetupSteps.tsx
Outdated
Show resolved
Hide resolved
client/web/src/setup-wizard/components/setup-steps/SetupSteps.tsx
Outdated
Show resolved
Hide resolved
client/web/src/setup-wizard/components/setup-steps/SetupSteps.tsx
Outdated
Show resolved
Hide resolved
| return createPortal( | ||
| <Button variant="primary" disabled={disabled} onClick={onNextStep}> | ||
| {label} | ||
| </Button>, | ||
| nextButtonPortalElement | ||
| ) |
|
|
||
| export const SetupWizard: FC = props => { | ||
| const [step, setStep] = useState(0) | ||
| const [activeStepId, setStepId, status] = useTemporarySetting('setup.activeStepId') |
There was a problem hiding this comment.
Also, I think we should add in logic about the last completed step to be the initial step. Imagine there's 5 steps, user is on step 3, goes back to view step 1, they exit, when they re-visit we should direct them to their latest complete step right?
This is also a data point we want to have saved to log for metrics, so could be good to have on hand. What do you think?
There was a problem hiding this comment.
Ideally yes, agree it would be best UX here, unfortunately I found a few problems with this and how we should work with browser history.
At the moment I implemented it in the way where we store the last visited step. It should be enough for the first implementation because steps on consumer level will be in charge of the Next button availability and this is how we prevent users from skipping required steps.
In the next PRs when we will have steps implementation I think we should add more logic and implement it as you described
user is on step 3, goes back to view step 1, they exit, when they re-visit we should direct them to their latest complete step
But imagine if user go back to the first step and did something that made this 1 step invalid. In this case we should redirect user to this 1 step next time when they hit root URL route "/setup"
There was a problem hiding this comment.
That makes sense, if a past step is re-visited and error'd we need to account for that. I'm just thinking how we can store furthest step visited, because at Merge we've been talking about logging at what point a user drops off the form if they don't complete for data collection. We can just store this separate from the routing logic too, though
| id: '003', | ||
| name: 'Sync repositories', | ||
| path: '/setup/sync-repositories', | ||
| render: () => <H2>Hello sync repositories step</H2>, |
There was a problem hiding this comment.
I can revise my work easily, just adding that component I was working on here. I can set my branch to merge into yours or wait for yours to merge
|
@st0nebraker so to summarise this PR logic. I was working on redirection logic and the last valid step logic and this is my thoughts on this
Ideally the logic should be
This might be difficult to understand which step is invalid (I actually not sure I have a clear understanding of step validity at the moment for case when steps are connected (like local/remote repositories steps)) I think we should simplify this logic about redirection and just open the last visited step if user go to the root URL "/setup" Proposal
|
One more thought on this: Should we add routing logic to re-direct if user enters url not in setup wizard? So we keep them in setup wizard url until it's complete. Because we want to force them to stay until complete, right? Maybe you've already thought of this or are saving to add this later. Thought I should raise it as it occurred to me. @vovakulikov |
|
This is a good catch @st0nebraker, I think we have an issue about this https://github.com/sourcegraph/sourcegraph/issues/46511. But I still not sure that blocking users here is a good idea, my opinion on this that we should provide ability to go back to the setup wizard but you also can just skip it and then when you realise that this is no much value in Sourcegraph without repositories you go back to the wizard. Side note here: I think that this redirection should live on the backend in order to avoid client-based routing complexity (if we decide keep users in setup wizard until they finished the required steps) |
This makes sense to me too, let them escape once in & have some sort of banner or reminder to finish setup to experience Sourcegraph better. Re: Re-direction logic, agreed initial re-direct living on the backend seems correct and less complex to me also. |
Closes https://github.com/sourcegraph/sourcegraph/issues/46641
This PR adds a new setup wizard stepper UI based on Figma designs here, Note that proper dark there support will be addressed later in the upcoming PRs (here is a comment about this in Figma)
Each step of the wizard has its own URL route, so if you refresh the page, you should see the current step. We sync the last valid active step in the temporary store, so if you were on /setup/ and open just /setup route, you should be redirected to /setup/ (the last valid step you were)
Test plan
setupWizardin your settings and go to the /setup route. Make sure that you see the first step in the wizard UIApp preview:
Check out the client app preview documentation to learn more.