Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Setup Wizard: add new setup steps UI#47384

Merged
vovakulikov merged 11 commits intomainfrom
vk/add-new-setup-steps-ui
Feb 8, 2023
Merged

Setup Wizard: add new setup steps UI#47384
vovakulikov merged 11 commits intomainfrom
vk/add-new-setup-steps-ui

Conversation

@vovakulikov
Copy link
Copy Markdown
Contributor

@vovakulikov vovakulikov commented Feb 3, 2023

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

  • Go to the /setup route and make sure that you see the repositories page UI
  • Turn on the experimental feature flag setupWizard in your settings and go to the /setup route. Make sure that you see the first step in the wizard UI
  • Check that you can go to the second step by clicking the Next button
  • Check that the second route renders a custom Next button, and it should be disabled
  • Make sure that if you on the first screen and you haven't seen any further steps in the setup wizard you can't open further steps by going to them by their URLs, you should be redirected to the last valid screen instead.

App preview:

Check out the client app preview documentation to learn more.

@vovakulikov vovakulikov self-assigned this Feb 3, 2023
@cla-bot cla-bot bot added the cla-signed label Feb 3, 2023
@vovakulikov vovakulikov force-pushed the vk/add-new-setup-steps-ui branch from 5671a77 to 95efd97 Compare February 3, 2023 18:08
@sg-e2e-regression-test-bob
Copy link
Copy Markdown

sg-e2e-regression-test-bob commented Feb 3, 2023

Bundle size report 📦

Initial size Total size Async size Modules
0.13% (+3.73 kb) 0.03% (+3.71 kb) -0.00% (-0.01 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits c0fe380 and a4799cb or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@vovakulikov vovakulikov marked this pull request as ready for review February 3, 2023 18:42
Copy link
Copy Markdown
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally ✅

Most of the comments are related to the ongoing react-router v6 migration. More context can be found here.

authenticatedUser === undefined ||
graphqlClient === undefined ||
temporarySettingsStorage === undefined ||
!settingsCascade
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we block rendering because of the missing settingsCascade value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +191 to +196
return createPortal(
<Button variant="primary" disabled={disabled} onClick={onNextStep}>
{label}
</Button>,
nextButtonPortalElement
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! 😎


export const SetupWizard: FC = props => {
const [step, setStep] = useState(0)
const [activeStepId, setStepId, status] = useTemporarySetting('setup.activeStepId')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vovakulikov
Copy link
Copy Markdown
Contributor Author

@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

  • At the moment each step has its own URL (like "setup/local-repositories" - first step for the app setup process, or "setup/remote-repositories" ... etc) and this is how we preserve wizard step if you refresh the page, step will be resolved by browser URL.
  • Even we have these steps as sequence of setup steps they are more like independent ordered set of steps rather then sequence when we're talking about the next step availability (see next point)
  • Imagine the situation when we skip the local step and we're on the second step, since we've skipped the local repositories step you have to connect some code host on this step to go to the next one
  • But if you provided local repositories paths on the first step then the second step is optional and this means it's automatically becomes valid? not sure.
  • Plus to this since each step is a separate page in terms of URLs and browser history if you're on the second step and you can click "back history button" in the browser and this redirects you to the prev visited step (this might be some further step if you previously clicked go to prev step in the wizard UI).
  • And the last one (ideally we should block all further step if the current step is invalid, but since it might be possible that one prev step is invalid but the next one is valid it might be difficult to track all of these cases) Imagine you are on the second step (and this step is valid you connected some code hosts) and you click back history button, you're on the first step when you did something that made this step invalid, we disable the next UI button in the wizard but what should we do with next history browser button? (we can't rewrite the history so it's still possible to go to the next step even your current step is invalid)

Ideally the logic should be

  • You can have access to only steps that are previous to the last valid step (you finished the first step and you are on the second it means that you can go to the first step URL and the second step URL but the third one is blocked because you have one step before the third one (the firs step) which is invalid
  • If you try to go to the third step by URL you have to redirected to the first invalid step URL
  • If you try to go to the root URL like "/setup" you should be redirected to the last invalid step (in process step)
  • But even in this logic there is a problem with go to there prev step, make it invalid, click next history browser button then what?

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

  • Just track your last visited step invalid step and restore it when someone try to go back to the wizard by "/setup" URL,
  • Do not block further steps by URL (it should be ok because all urls is unique so I doubt that someone will be able to navigate to the further steps without prev steps completion)
  • Block further steps only by setup wizard next button

@vovakulikov vovakulikov merged commit e411b9a into main Feb 8, 2023
@vovakulikov vovakulikov deleted the vk/add-new-setup-steps-ui branch February 8, 2023 15:17
@st0nebreaker
Copy link
Copy Markdown
Contributor

Proposal
Just track your last visited step invalid step and restore it when someone try to go back to the wizard by "/setup" URL,
Do not block further steps by URL (it should be ok because all urls is unique so I doubt that someone will be able to navigate to the further steps without prev steps completion)
Block further steps only by setup wizard next button

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

@vovakulikov
Copy link
Copy Markdown
Contributor Author

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)

@st0nebreaker
Copy link
Copy Markdown
Contributor

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.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement stepper component for the setup wizard UI

4 participants