react-step-progress icon indicating copy to clipboard operation
react-step-progress copied to clipboard

Feature Request: Declarative Step Content Switching

Open r-bt opened this issue 4 years ago • 6 comments

Proposal

Switch from passing reference to React Component as content in ProgressStep object to providing a component which only renders it's children when it's step matches the current step.

Example

return (
      <ProgressBar />
      <StepProgress>
         <Step step={1}>
             <h1> First step </h1>
         </Step>
         <Step step={2}>
             <h1> Second step </h1>
         </Step>
      </StepProgess>
);

Other

This is similar to how react-router currently works and allows for component composition along with better readability (in my opinion). I'm not sure on the names of the components so I'm open to suggestions. Again, if this is something you'd be interested in incorporating I'll submit a pull request

r-bt avatar Jul 01 '20 11:07 r-bt

I've spun up a simple version of this, which needs some refactoring but has the main elements.

r-bt avatar Jul 01 '20 14:07 r-bt

Was busy so didn't get time to look into it, will probably go through this over the weekend

saini-g avatar Jul 03 '20 05:07 saini-g

No bother, there's no rush.

r-bt avatar Jul 03 '20 08:07 r-bt

@richardbeattie Thanks for the help on this. I went through your code and here are my thoughts I really like the way you have separated out the components and the hooks, one question though, will this be usable inside a class based component

I also liked the analogy you made with react-router, but IMO this introduces unnecessary complexity for the new developer both when contributing and using. Just like you handled this case in your code, this is the exact reason I feel that it is unnecessary to ask anyone using this component to first import the Step and the StepProgressBar and then use these in their markup. For example: like you did in this example

const steps = [...];

const { stepForward, stepBackwards, getProps, currentIndex } = useStepProgress({ steps, startingStep: 0 });

<StepProgress {...getProps} >
  
  // this is always going to be the same, so it can be removed from here and rendered internally
  <StepProgressBar />
  
  // supplying the content like before and letting the component handle the rendering of the steps internally
  // this frees the users of any concerns regarding implementation details
  <Step step={1}>
      <h1>Hello there</h1>
  </Step>
  ...
</StepProgress>
<div className="step-buttons">
  // Previous and Next buttons with their onClick handlers
</div>

One last thing, I was planning on allowing users to have separate actions to be performed on each step, your approach as mentioned above kind of puts a limitation on the click handlers of the next and previous buttons to be the same for each step.

saini-g avatar Jul 05 '20 13:07 saini-g

Hey @saini-g, finally got around to the project I was needing this for sorry for the long delay :) and thanks for looking through this.

one question though, will this be usable inside a class based component

This would not be usable in class components, for me I'm using hooks everywhere but yeah if you want to support class components this won't work, if so that's no bother.

but IMO this introduces unnecessary complexity for the new developer both when contributing and using.

I can definitely see how adding more components would add to the complexity. However, conversely sticking more closely to react's philosophy of declarative systems may also help with moving between libraries and design thoughts. Personally, I found this library with not a lot of react knowledge and having to pass components to the props rather than composing them felt odd. However, I imagine there's a lot of personal preference

In regards to rendering StepProgressBar explicitly, this allows for greater flexibility and style customization. For instance, I'm currently using this with styled-components so I am able to make a custom Progress Bar Component. Another place would be say if you wanted a vertically bar and needed to place it in a flex container.

your approach as mentioned above kind of puts a limitation on the click handlers of the next and previous buttons to be the same for each step.

With this way and the stepForward & stepBackwards hooks from #3 I'm able to do this quite nicely actually for instance it'd look somewhat like


const handleFirstForward = () => {
   // Do anything you need to do
  stepForward();
}

const handleSecondForward = () => {
   // Do anything you need to
   stepForward();
}

<StepProgress {...getProps} >

  <StepProgressBar />
  
  <Step step={1}>
      <button onClick={handleFirstForward}>Next</button>
  </Step>
  <Step step={2}>
       <button onClick={handleSecondForward}>Next</button>
       <button onClick={stepBackwards}>Back>Back</button>
  </Step>
  ...
</StepProgress>

Anyway, if this isn't something you're interested in that's absolutely no bother

r-bt avatar Dec 16 '20 20:12 r-bt

I would just like to add that the original code was great and easy to use (thanks @saini-g ), BUT as to what r-bt said, feeding the step with a react component and state into this child component broke the method.

Thanks @r-bt for your work as it made this module useable for my use case.

To clarify, this is using React useState hooks in the parent:

const step1Content = <Component hello={hello} setHello={setHello} />;

when inside

<StepProgressBar startingStep={0} onSubmit={onFormSubmit} steps={[ { label: 'Step 1', subtitle: '10%', name: 'step 1', content: step1Content }, { label: 'Step 2', subtitle: '50%', name: 'step 2', content: step2Content, validator: step2Validator }, { label: 'Step 3', subtitle: '100%', name: 'step 3', content: step3Content, validator: step3Validator } ]} />;

Does not let the child update the parent state. Therefore the modification made by @r-bt allows this functionality to work as intended.

Therefore @saini-g I think allowing this pr would actually make it more user friendly. At the moment I have to use the forked version. But thanks again for both your work, saved me a bunch of time either way and looks great!.

nicholasdavidbrown avatar Feb 10 '21 07:02 nicholasdavidbrown