Router multi step forms #1966

mudrila · 2024-05-30T23:14:08Z

This PR contains following changes:

Recompose DAO Creation flow, Proposal Creation flow and Proposal Template Creation flow into navigation-based multi-step form, each step is reflected in the URL
This leads to smoother experience when using browser built-in "back" functionality - instead of actually taking your to the previous page, from which you came to the proposal builder - it takes you to the previous step of the form itself
Also prevent user from selecting governance type and switching to the network which doesn't support it

For testing:

Go to the DAO creation while being on Sepolia
Notice URL now is "create/essentials" and not just "create"
Select "NFT Voting"
Switch network to "mainnet"
Notice toast appears saying "Nope and blah blah"
Fill in DAO name and go to the next step - notice url change
Try using browser's "Go Back" button
Notice you're taken to the previous form step, not the previous page you came from
Verify scroll to top happens after going to the next/prev step if you're on smaller screens (mobile) and you've scrolled couple window heights down

Closes #1914
Closes #1915

@adamgall I slightly altered the behavior comparing to what you mentioned in the ticket - instead of letting user know when he already filled in everything required - I'm immediately taking him to the first step and showing toast that certain governance type is not supported on current network. Think it is gonna be hella frustrating to input all the NFTs you have and then you get "Oopsie, not supported".

netlify · 2024-05-30T23:14:29Z

✅ Deploy Preview for decent-interface-dev ready!

Name	Link
🔨 Latest commit	`6883af6`
🔍 Latest deploy log	https://app.netlify.com/sites/decent-interface-dev/deploys/667077ad5fb20f0008788485
😎 Deploy Preview	https://deploy-preview-1966.app.dev.decentdao.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

adamgall · 2024-05-31T00:24:31Z

I've been doing a bit of manual testing, and so far the following things would need to be "fixed" before we can consider merging:

"scroll to top" behavior has been lost, we unfortunately don't get that by default when changing routes.
Reloading the page while in the middle of the Safe creation flow loses prior state, but keeps the user on their current step. This results in the "Deploy" button not working. I don't see any errors in the console (which is also a code smell -- that means the code is probably just checking for existence of state and not yelling if it's not there. The code should yell.)
- My recommendation is that if the route is loaded and the correct state that should be there at this point in the flow isn't there, redirect the user back to the beginning.

mudrila · 2024-05-31T06:37:08Z

I've been doing a bit of manual testing, and so far the following things would need to be "fixed" before we can consider merging:

"scroll to top" behavior has been lost, we unfortunately don't get that by default when changing routes.

Reloading the page while in the middle of the Safe creation flow loses prior state, but keeps the user on their current step. This results in the "Deploy" button not working. I don't see any errors in the console (which is also a code smell -- that means the code is probably just checking for existence of state and not yelling if it's not there. The code should yell.)

My recommendation is that if the route is loaded and the correct state that should be there at this point in the flow isn't there, redirect the user back to the beginning.

On the missing "scroll to top" behaviour - I've seen scroll is happening but only when you already scrolled down at least one screen, otherwise scroll position persists. I do agree it will be better if it will just scrolled to top always, gonna add this change
Agree, I'll add this logic. With proposal builder there's couple of issues with that, though:

Title/description aren't actually necessary for Multisig but we do validate that title is there. I'm gonna fix it
Something aside of this PR - we don't have documentation URL on proposal builder at all. I'm gonna open up another PR for this, should be an easy fix

…s, navigate to essentials from multisig if DAO name or governance type is not set

…t set

…neering

mudrila · 2024-05-31T07:46:40Z

I've been doing a bit of manual testing, and so far the following things would need to be "fixed" before we can consider merging:

"scroll to top" behavior has been lost, we unfortunately don't get that by default when changing routes.

Reloading the page while in the middle of the Safe creation flow loses prior state, but keeps the user on their current step. This results in the "Deploy" button not working. I don't see any errors in the console (which is also a code smell -- that means the code is probably just checking for existence of state and not yelling if it's not there. The code should yell.)

My recommendation is that if the route is loaded and the correct state that should be there at this point in the flow isn't there, redirect the user back to the beginning.

@adamgall Done, check updated implementation pls

adamgall · 2024-05-31T14:15:20Z

Thanks for the improvements @mudrila! Some more suggestions:

Don't let /create result in a 404. Please redirect it to /create/essentials.
All of these various scrollToTop function calls are kinda lame. Can we fix this once and for all with something like this?
```
import { useEffect } from 'react';
import { useLocation } from 'react-router-dom';

export default function ScrollToTop() {
  const { pathname } = useLocation();

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [pathname]);

  return null;
}
```
and then in Layout, render this new ScrollToTop component right above the top-level Grid. Also I'd love it if it didn't "scroll", but just "jumped" and we didn't even notice it.

mudrila · 2024-05-31T15:49:29Z

Thanks for the improvements @mudrila! Some more suggestions:
Don't let /create result in a 404. Please redirect it to /create/essentials.
All of these various scrollToTop function calls are kinda lame. Can we fix this once and for all with something like this?
import { useEffect } from 'react';
import { useLocation } from 'react-router-dom';

export default function ScrollToTop() {
  const { pathname } = useLocation();

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [pathname]);

  return null;
}
and then in Layout, render this new ScrollToTop component right above the top-level Grid. Also I'd love it if it didn't "scroll", but just "jumped" and we didn't even notice it.

@adamgall Instead of component I just made a hook - I think "Component that returns null and calls hook" is kinda anti-pattern - if all it does is calling hook - just call damn hook in the place where you were rendering that empty component

adamgall · 2024-05-31T16:01:49Z

Thanks for the improvements @mudrila! Some more suggestions:
Don't let /create result in a 404. Please redirect it to /create/essentials.
All of these various scrollToTop function calls are kinda lame. Can we fix this once and for all with something like this?
import { useEffect } from 'react';
import { useLocation } from 'react-router-dom';

export default function ScrollToTop() {
  const { pathname } = useLocation();

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [pathname]);

  return null;
}
and then in Layout, render this new ScrollToTop component right above the top-level Grid. Also I'd love it if it didn't "scroll", but just "jumped" and we didn't even notice it.
@adamgall Instead of component I just made a hook - I think "Component that returns null and calls hook" is kinda anti-pattern - if all it does is calling hook - just call damn hook in the place where you were rendering that empty component

Haha, good point

tomstuart123 · 2024-06-03T13:06:31Z

Hey @mudrila / @adamgall , love to see this coming down the line.

So I just tested two use cases:

Create DAO flow - ✅ all worked perfect
Create ChildSafe flow - ❌ - appears broken by this new

See video for full experience of (2) here - https://decentdao.slack.com/archives/C06QZTUTKRC/p1717419840496079?thread_ts=1717419826.480139&cid=C06QZTUTKRC

This is the main issue at 0:40 seconds. When clicking 'create child safe' the URLs get mixed between the new 'essentials' and childsafe URL

Note - At 0:18 and 1:00 I also got errors when navigating back to a DAO after starting the create DAO flow. This could be a seperate issue but wanted to raise it in case it was URL related.

Da-Colon

Create DAO ✅
Create ChildDAO ❌

Same error as @tomstuart123

Proposal Create Proposals: ✅

Proposal Templates ❌

Navigating to Templates via Create button on page (see image)

src/components/DaoCreator/StepController.tsx

mudrila · 2024-06-10T14:07:36Z

(Edge Case) Forward Button shenanigans -> Esstentials -> Governance (Token DAO) -> Token Details -> Hit back -> Change to Multisig -> Hit Browser Forward

Takes you to token details but the setting is not Multisig

(Bug) Hitting Back When viewing Create Child DAO -> Click Add a child safe -> Hit Back

This takes you to Decent Home page instead of the DAO Dashboard

good catches @Da-Colon. Clearly I needed to do even more clicking around

That's probably new issue introduced after very latest changes.
@Da-Colon TY! I'll fix these

adamgall · 2024-06-10T15:51:37Z

(Edge Case) Forward Button shenanigans -> Esstentials -> Governance (Token DAO) -> Token Details -> Hit back -> Change to Multisig -> Hit Browser Forward

Takes you to token details but the setting is not Multisig

(Bug) Hitting Back When viewing Create Child DAO -> Click Add a child safe -> Hit Back

This takes you to Decent Home page instead of the DAO Dashboard

Team agreed (on standup) that first "issue" here is expected behavior and not something to fix.

mudrila · 2024-06-10T15:56:31Z

(Bug) Hitting Back When viewing Create Child DAO -> Click Add a child safe -> Hit Back

This takes you to Decent Home page instead of the DAO Dashboard

@Da-Colon @adamgall Figured out this bug was there for a while lol. Fixed 🚀

DarksightKellar

Finally finished this code review, whew. I'll leave code-level approval for someone else on the dev team, but code looks good to me as of now. I'll get around to manual testing tomorrow, which I have not yet done.

src/components/ProposalBuilder/index.tsx

src/components/ProposalBuilder/StepButtons.tsx

src/components/DaoCreator/StepController.tsx

adamgall

I think the code used to determine nextStep and previousStep could be improved.

I think that improvement starts with where the enum CreatorSteps are being defined. That enum doesn't actually define a "set of steps", it defines a bunch of independent steps, but there's no relationship between them.

Starting from the type definition code here, can we actually create a set of (numbered) enums, one for each type of DAO which might be created.

For example:

enum RootMultisigSteps {
  ESSENTIALS,
  MULTISIG_DETAILS,
}

enum ChildMultisigSteps {
  ESSENTIALS,
  MULTISIG_DETAILS,
  FREEZE_DETAILS,
}

enum RootERC20Steps {
  ESSENTIALS,
  ERC20_DETAILS,
  AZORIUS_DETAILS,
}

These enums all have numbers as their base type, and importantly the different "steps" within them are numbered, so it should be much easier to know which step is "prev" and which is "next", when we're in the actual step code flow.

Will something like this work? Let me know if it would be easier to talk out in person.

DarksightKellar · 2024-06-12T12:49:48Z

Bug?

This url: "https://deploy-preview-1966.app.dev.decentdao.org/create/essentials/" (trailing slash) triggers a weird recursive redirect loop that ends up sth like this (note address bar content):

DarksightKellar · 2024-06-12T14:38:14Z

I think in general there's a slight problem with how trailing slashes are handled (add a slash to practically any valid url within the app):

This isn't happening on develop

adamgall

I'm gonna stop my code review prematurely, and talk about more structural changes I'd like to see in this PR.

Now that we've got a good framework for dynamically rendering UI, let's reorganize the structure of our routes to bring some closeness (and maybe convergence) between the different "dao creation flavors" (arrays of enum values), and the url structure.

You might see where I'm going with this.

new root ERC20 token dao on the first step
- /create/root/erc20/essentials
new root ERC20 token dao on the second step
- /create/root/erc20/erc20token
new root ERC20 token dao on the third step
- /create/root/erc20/azorius
new child multisig dao on the first steps
- /create/child/multisig/essentials
... etc

I expect that continued experimentation in this direction will lead to some pretty code.

Specifically, i'd like to see the URL structure uniquely identify any step in any creation flow.

adamgall · 2024-06-14T18:22:21Z

src/components/DaoCreator/StepButtons.tsx

  const { t } = useTranslation(['daoCreate', 'common']);
  const {
    readOnly: { user },
  } = useFractal();
+  const location = useLocation();
+  const paths = location.pathname.split('/');
+  const step = (paths[paths.length - 1] || paths[paths.length - 2]) as CreatorSteps | undefined;


Can you leave a comment in code explaining why there are two options for "current step"

btw, what was the outcome of trying to "remove trailing slashes" that was a fkn nightmare the other day?

Oh yeah - freaking nightmare, I just gave up and made it to work with trailing slash with this "hacky" way 🤣

src/components/DaoCreator/StepButtons.tsx

…ps defined

adamgall

I've gotten through this review, and it's a lot of great work.

I just feel a kind of way about how StepController.tsx is structured. I wonder why it can't flow more smoothly?

Early this week let's keep talking about this PR.

src/constants/routes.ts

src/components/DaoCreator/hooks/useStepRedirect.ts

src/types/createDAO.ts

adamgall · 2024-06-17T02:31:05Z

src/components/DaoCreator/StepButtons.tsx

  const { t } = useTranslation(['daoCreate', 'common']);
  const {
    readOnly: { user },
  } = useFractal();
+  const location = useLocation();
+  const paths = location.pathname.split('/');
+  const step = (paths[paths.length - 1] || paths[paths.length - 2]) as CreatorSteps | undefined;


btw, what was the outcome of trying to "remove trailing slashes" that was a fkn nightmare the other day?

src/components/ProposalBuilder/StepButtons.tsx

adamgall · 2024-06-17T02:43:51Z

src/components/ui/page/Layout/index.tsx

+  useNavigationScrollReset();
+


Might be cool to pull this sub-feature, and maybe the changes to "scrollToTop" and "scrollToBottom" into a new PR.

Anything else that can be broken off of this into a new PR?

Well these changes just ~15-20 lines of code - I don't see much benefits of extracting that into separate PR, unless you think that's reasonable. They also contextually bound to this PR as initially solving the problem that scrolling is not happening while you navigating between steps of proposal builder

…posalBuilder

…describe why prevStepUrl and nextStepUrl are calculated this way in StepButtons from ProposalBuilder

adamgall · 2024-06-17T18:05:19Z

src/components/DaoCreator/StepController.tsx

-    setStepState(newStep);
-    scrollToTop();
-  };
+function StepController(props: Omit<ICreationStepProps, 'steps'>) {


Can we not omit steps from this object, and use it in place of the const steps = useMemo() below?

Not really, coz we aren't passing steps prop here actually - this is the place where it is calculated first time and I felt like it would be right place to do so comparing to the moment of rendering

Since steps are calculated based off values.governance - this is the highest component where we can leverage useMemo, otherwise we'll be recalculating them during each rendering of StepController

So I feel like we should leave it as is

I'm sorry, but no, the more I look at this StepController the more I want to see this implemented differently.

The things I'm thinking about:

Pass the steps in as a prop

I don't like how these Routes are being declared, conditionally like this.

I will take a stab at implementing the direction I'm thinking.

Why would we be passing steps as prop here though? This is literally StepsController component which I would expect to see this stuff
Creating some wrapper would be an overkill IMO
Conditional routes is an over-engineering, totally agree

@adamgall Id rather create proper type definition

Not another wrapper, no. But, at some point the user clicks some button which triggers these steps components to mount and render. At the place of that button click can't we know which set of steps the user is intending to do, add pass in the appropriate CreatorSteps object from right there at that button click?

Very likely that I'm incorrectly assuming how things work in other parts of the codebase, because what I described above seems like the obvious way to design these changes in this PR.

mudrila added 2 commits May 31, 2024 00:38

Turn DAO Creation form into router-based implementation

5a8a68e

Refactor ProposalBuilder into Route-based form

60e1946

mudrila added bug Something isn't working enhancement New feature or request maintenance Keep the lights on labels May 30, 2024

mudrila requested review from azadikoff, adamgall, DarksightKellar, xraystyle1980, tomstuart123, Da-Colon, nicolaus-sherrill and xanaramoss May 30, 2024 23:14

mudrila self-assigned this May 30, 2024

mudrila added 4 commits May 31, 2024 09:37

Add scrolling while navigating forth and back between multi-step form…

6cac95a

…s, navigate to essentials from multisig if DAO name or governance type is not set

Redirect on DAO creation steps when dao name or governance type is no…

f07594e

…t set

Add comment, explaining that I'm not lazy but just avoiding over-engi…

7423cd7

…neering

Prettier

46c9744

Add useNavigationScrollReset

b2a0021

Da-Colon suggested changes Jun 3, 2024

View reviewed changes

src/components/DaoCreator/StepController.tsx Outdated Show resolved Hide resolved

mudrila added 2 commits June 4, 2024 17:35

Fix subDAO creation

a485807

Remove dumbass counter :(

e0b51d7

Fix bug with clicking back button on Create subDAO

e2ff50d

mudrila requested a review from Da-Colon June 10, 2024 15:56

DarksightKellar reviewed Jun 10, 2024

View reviewed changes

src/components/ProposalBuilder/index.tsx Outdated Show resolved Hide resolved

src/components/ProposalBuilder/StepButtons.tsx Outdated Show resolved Hide resolved

src/components/DaoCreator/StepController.tsx Outdated Show resolved Hide resolved

adamgall suggested changes Jun 11, 2024

View reviewed changes

mudrila added 5 commits June 14, 2024 17:46

Merge branch 'develop' into feature/router-multi-step-forms

0fa7271

Fix redirect loop

a7211a8

Rename StateButtons -> StepButtons

c60d2bd

Fix redirect loop

35e887f

Keep search in Navigate

897f2af

mudrila requested a review from DarksightKellar June 14, 2024 17:32

Clearly define routes through constant arrays

47bcb7e

mudrila requested a review from adamgall June 14, 2024 18:19

adamgall suggested changes Jun 14, 2024

View reviewed changes

Do not render routes when steps are not including respective form ste…

a4f81ef

…ps defined

mudrila requested a review from adamgall June 14, 2024 20:30

adamgall reviewed Jun 17, 2024

View reviewed changes

mudrila added 3 commits June 17, 2024 17:44

Remove extracted StepButtonsContainer and just use Flex

f097f01

Remove some unused DAO_ROUTES since they're covered internally by Pro…

983cf40

…posalBuilder

Remove overcomplicated types for CreatorSteps and add dev comment to …

6883af6

…describe why prevStepUrl and nextStepUrl are calculated this way in StepButtons from ProposalBuilder

mudrila requested a review from adamgall June 17, 2024 17:54

adamgall reviewed Jun 17, 2024

View reviewed changes

adamgall approved these changes Jun 18, 2024

View reviewed changes

Da-Colon approved these changes Jun 18, 2024

View reviewed changes

mudrila merged commit 7b4fb4c into develop Jun 18, 2024
7 checks passed

mudrila deleted the feature/router-multi-step-forms branch June 18, 2024 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Router multi step forms #1966

Router multi step forms #1966

mudrila commented May 30, 2024 •

edited

Loading

netlify bot commented May 30, 2024 •

edited

Loading

adamgall commented May 31, 2024

mudrila commented May 31, 2024

mudrila commented May 31, 2024

adamgall commented May 31, 2024

mudrila commented May 31, 2024

adamgall commented May 31, 2024

tomstuart123 commented Jun 3, 2024

Da-Colon left a comment •

edited

Loading

mudrila commented Jun 10, 2024

adamgall commented Jun 10, 2024

mudrila commented Jun 10, 2024

DarksightKellar left a comment

adamgall left a comment

DarksightKellar commented Jun 12, 2024

DarksightKellar commented Jun 12, 2024

adamgall left a comment •

edited

Loading

adamgall Jun 14, 2024

mudrila Jun 14, 2024

adamgall Jun 17, 2024

mudrila Jun 17, 2024

adamgall left a comment

adamgall Jun 17, 2024

adamgall Jun 17, 2024

mudrila Jun 17, 2024

adamgall Jun 17, 2024

mudrila Jun 18, 2024

adamgall Jun 18, 2024

mudrila Jun 18, 2024

mudrila Jun 18, 2024

adamgall Jun 18, 2024

Router multi step forms #1966

Router multi step forms #1966

Conversation

mudrila commented May 30, 2024 • edited Loading

netlify bot commented May 30, 2024 • edited Loading

✅ Deploy Preview for decent-interface-dev ready!

adamgall commented May 31, 2024

mudrila commented May 31, 2024

mudrila commented May 31, 2024

adamgall commented May 31, 2024

mudrila commented May 31, 2024

adamgall commented May 31, 2024

tomstuart123 commented Jun 3, 2024

Da-Colon left a comment • edited Loading

Choose a reason for hiding this comment

mudrila commented Jun 10, 2024

adamgall commented Jun 10, 2024

mudrila commented Jun 10, 2024

DarksightKellar left a comment

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

DarksightKellar commented Jun 12, 2024

DarksightKellar commented Jun 12, 2024

adamgall left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudrila commented May 30, 2024 •

edited

Loading

netlify bot commented May 30, 2024 •

edited

Loading

Da-Colon left a comment •

edited

Loading

adamgall left a comment •

edited

Loading