Page MenuHomePhabricator

[web] Introduce `Stepper` - common multiple step component
ClosedPublic

Authored by jakub on Sep 20 2022, 1:20 AM.
Tags
None
Referenced Files
F3572791: D5186.id16947.diff
Sat, Dec 28, 12:54 PM
Unknown Object (File)
Fri, Dec 27, 8:41 AM
Unknown Object (File)
Fri, Dec 27, 8:41 AM
Unknown Object (File)
Fri, Dec 27, 8:41 AM
Unknown Object (File)
Fri, Dec 27, 8:41 AM
Unknown Object (File)
Fri, Dec 27, 8:41 AM
Unknown Object (File)
Fri, Dec 27, 8:41 AM
Unknown Object (File)
Fri, Dec 27, 8:41 AM

Details

Summary

Depends on D5185
Context: here

Introduce common Stepper component, for creating multiple step forms and (in the future) multiple page modals.

The component receives steps, information about active step and steps setter from parent - it doesn't keep the state with current active step itself.

For better code cleanliness, to set activeStep, we use children name property instead of using array indexes.

Test Plan

This component will be used in next diff (firstly in ComposeSubchannelModal). Currently, no changes in the app.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jakub edited the test plan for this revision. (Show Details)
jakub added reviewers: tomek, atul.
jakub edited the summary of this revision. (Show Details)
jakub edited the test plan for this revision. (Show Details)

Rebase

Add optional LoadingIndicator to buttons

@jakub, is this diff ready to review, or are you still working on amending it? If you're still working on amending it, please hit "Plan Changes" to remove it from your reviewers' queues.

Move error message to left side

tomek requested changes to this revision.Sep 26 2022, 4:50 AM
tomek added inline comments.
web/components/stepper.css
2–9 ↗(On Diff #17016)

It is a bad idea to have a scrollable component within another scrollable component - this approach can create visual issues, which you probably have noticed.

41 ↗(On Diff #17016)

Having to set a height for a text sounds strange. It should be enough to set line height and position the text using flex.

web/components/stepper.react.js
11 ↗(On Diff #17016)

Can we make it more general and use React.Node?

29–56 ↗(On Diff #17016)

This function doesn't depend on props so maybe it is a good idea to move it outside the component?

36–39 ↗(On Diff #17016)

Please check how loading state was implemented in add-users-list.react.js. In that implementation the size of loading / loaded state remains the same and we don't need to use absolute positioning.

48 ↗(On Diff #17016)

Are spaces inside div added by a formatter?

93 ↗(On Diff #17016)

Why do we use substring(2)?

97 ↗(On Diff #17016)

Memoization doesn't work when there are multiple children - it is an array that gets generated in each render. There's no good solution to this problem, so it's better to not use memo at all to avoid confusion.

104 ↗(On Diff #17016)

What's the purpose of this code? Maybe we can express the same intention in a simpler way?

This revision now requires changes to proceed.Sep 26 2022, 4:50 AM
jakub marked 7 inline comments as done.

Adjust to code review: simplify code, move prepareButton function outside StepperItem
component, remove unnecessary css prop

web/components/stepper.css
41 ↗(On Diff #17016)

I tried to avoid additional resizing, when the error message is empty like on videos below:

With fixed size;

Without fixed size:

web/components/stepper.react.js
93 ↗(On Diff #17016)

By default, children keys contains an additional .$ prefix. I use substring(2) to skip it, but alternatively, we could add this prefix to every Map.get() call instead
(something like this:

const activeComponent = children[index.get(`.\$${activeStep}`) ?? 0];

)

tomek requested changes to this revision.Sep 27 2022, 5:01 AM
tomek added inline comments.
web/components/stepper.css
41 ↗(On Diff #17016)

Ah, that makes sense. Could you check how this issue was solved in other modals?

web/components/stepper.react.js
18–21 ↗(On Diff #17094)

User readonly props

93 ↗(On Diff #17016)

Sounds like a really risky solution - this might be an implementation detail that can change in any update. Looking at other similar solutions, e.g. react router uses custom prop path. Maybe we should also do something similar and use our own property instead of using built-in key?

This revision now requires changes to proceed.Sep 27 2022, 5:01 AM
jakub added inline comments.
web/components/stepper.css
41 ↗(On Diff #17016)

I check how it looks in add-users-list.react.js. Error message here only appears when is needed and from visual aspect it looks like on second video. I could adjust it here.

Adjust to code review:

  • Change ActionButtonsProps to readonly props
  • use new name prop instead of using key
  • remove errorMessage fixed size
tomek requested changes to this revision.Sep 28 2022, 8:07 AM

Looks great! Only some minor issues mentioned in comments.

web/components/stepper.css
41 ↗(On Diff #17016)

Maybe we should also add a margin between the buttons and the list - to make things consistent with other places

web/components/stepper.react.js
74 ↗(On Diff #17134)

Please remove additional spaces

92–96 ↗(On Diff #17134)

We can simplify it a little more by storing components directly in index instead of array indices

99 ↗(On Diff #17134)

We should prefer using classNames library for merging classes

This revision now requires changes to proceed.Sep 28 2022, 8:07 AM
jakub marked 3 inline comments as done.

Adjust to code review:

  • add margin to gap between buttons and the step content
  • remove additional spaces
  • simplify code
  • use classnames instead of string literals
This revision is now accepted and ready to land.Sep 29 2022, 6:21 AM