@manishakanyal I don't think hiding the "done" steps is the best choice here. I'm not sure what the precise description of the idea, you have, was, but (at least from my POV), the goal here is something of a smoother experience.

If the steps just disappear, we could have gone with the same 'one step at a time, just click when you're done' style of the rest of the wizard, but the intent of suddenly changing the UX from "horizontal" to "vertical" is to be able to keep the information in context.

Disregarding the UI looks (which we will be focusing on later), the feel of this particular page is supposed to be in the spirit of a "Scrolldown webpage", where you see the card/box with the actual step, but the next one's header "just lurks on the bottom of the page".

Now our texts will almost certainly be shorter than "a whole page", so having a small box in the middle of the page, and keeping the rest would look & feel bad. To combat that, a little bit, we came up with the concept of making the "current" step the most pronounced, while keeping the next steps (and possibly even the previously completed ones) rather transparent & desaturated, to keep the eye from wandering too much.

So, the initial PoC I'd like to see is:

• All the steps are one one page
• The current step is opaque and saturated to catch the eye
• The next steps are transparent and desaturated, Ideally also somewhat vertically spaced down from the current step
• When the current step is completed by the user - indicated by either clicking some "I'm done" style button or scrolling down:
  • The Current step's header gets a colored tint, the box gets slightly less opaque.
  • The page scrolls in such a way, that it snaps to the Next step, so it is shown on top of the page, letting a bit of the Current step lurking from the top.
  • The Next step becomes opaque and saturated, becoming the new Current step
• When scrolling up, the reverse effect happens

Hope that makes sense. I'd rather not spend a day creating an animation in Gimp/Inkscape to show exactly what I mean, so fire up with any questions you might have.

Okay got it, I'll implement the changes!

Hi @jskladan, @lbrabec I have implemented the changes, please review them.

Hi Manisha,

there is some disconnect between what you implemented and what Josef described. Therefore, unfortunately, I must ask you to take a step back and don't use the last commit. Base your next work on the initial commit in this pull request, which was closer (if unrefined) to the description that Josef posted.

While I can see, how you could have interpreted it in a "maybe sounds like a vertical carousel" fashion, that is not what we meant by "in the spirit of a scrolldown webpage".

The intent, once again, is to gradually add/show the information about the steps to be taken, keeping track of what was done, and hinting/showing that there are more steps to be followed.

Please let us know where and how exactly we can add more to the description, and what was unclear to you, so we can get to a result we're happy with.

@lbrabec Can you please give me any live demo or example, sorry I'm unable to understand!

@manishakanyal Could you please be specific, and describe in reasonable detail, what exactly you don't understand?

We appreciate the difficulty of on-line cooperation, so help us help you get to the desired outcome, without either me or @lbrabec implementing it instead of you.

Let's use the 'step-by-step' specification of the required PoC to pinpoint and describe where exactly you feel lost/unable to understand. Posting below for reference:

• All the steps are one one page
• The current step is opaque and saturated to catch the eye
• The next steps are transparent and desaturated, Ideally also somewhat vertically spaced down from the current step
• When the current step is completed by the user - indicated by either clicking some "I'm done" style button or scrolling down:
• The Current step's header gets a colored tint, the box gets slightly less opaque.
• The page scrolls in such a way, that it snaps to the Next step, so it is shown on top of the page, letting a bit of the Current step lurking from the top.
• The Next step becomes opaque and saturated, becoming the new Current step
• When scrolling up, the reverse effect happens

@jskladan, So, basically, All steps which are current and after current will be visible(opaque and translucent). Previous steps will not be shown in any case(until the user intends to)?
In the current implementation, all the steps are being hidden behind the current step(apart from the previous and next one(top and bottom).

@jskladan, The implementation that I can think of is this? Please let me know what needs to be changed.
Ref: Screenshot

@manishakanyal have a look at this short animation https://www.dropbox.com/s/joc981uzffcxpcg/Video%2028.06.2021%2012%2043%2053.mov?dl=0 that shows (in an unrefined way) what it is we want, and hopefully answers your questions.

Please implement this as a MVP, from which we'll be honing the look&feel of the UI.

Thanks!

Thanks a lot, @jskladan, all my doubts are cleared now :)

@jskladan, @lbrabec I have implemented PoC for this, please have a look.

I think you're heading the right direction!

I'm not sure I like when you get to the guide, the page is scrolled down to the first step. Could you please make the app scroll to a particular step only when the step is in URL? Right now, when you get to the guide through the wizard, the first URL is /.../beginner_guide. When you go to step 2 and pres previous the URL is /.../beginner_guide?tab=box1. So there is already a distinction, could you only scroll when ?tab=boxX is present?

@jskladan WDYT?

I'm sorry I didn't notice that, sure thing I'll fix it!

Apart of what @lbrabec said, I think it's a fine PoC. Next up, let's add some bling to it :)

In no particular order:

• Add bottom margin to the active step to make it a bit more spatial
• Ensure there is enough space after the last step to be able to scroll in such a way that it's on the top of the page
• I don't think the 'Previous' button is necessary, Move the next button to the right, and replace it with an ✔ button, that is grey at the beginning, and changes color to green, once clicked (also moves to the next step, as the current 'Next' button does)
• Add a visual difference to the "completed" steps (e.g. greenish tint to the header)
• Make the completed steps more opaque, still keeping them visually distinctly "in the background" but easier to read
• The page's content (it is the same in all the other places, but really shows here) is narrower than the breadcrumbs box, make the content as wide as the breadcrumbs

In an ideal world, I'd like for the completed steps to get opaque when an user scrolls up on to them. I guess that would be a bit tricky, but have a go, and look around for some libraries/components that might work well with visibility.

There might be more, but this should at least get you started.

Okay I'll implement the changes

Hi @lbrabec, @jskladan,

I can only think of this approach as of now which is not working only two cards are showing I'm unable to figure out what's wrong, it would be great if I can get some help or some libraries.

I tried to google a bit and found Intersection Observer API. You could use this custom hook:

function useOnScreen(ref) {
  const [isIntersecting, setIntersecting] = useState(false)
  const observer = new IntersectionObserver(
    ([entry]) => {
      setIntersecting(entry.intersectionRatio > 0.95)
    },
    {
      threshold: [0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0],
    }
  )
  useEffect(() => {
    observer.observe(ref.current)
    return () => { observer.disconnect() }
  }, [])
  return isIntersecting
}

Use it this hook in function Tab:

const ref = useRef()
const isVisible = useOnScreen(ref)
return (
    <div ref={ref}>
        // ... what currently return in Tab
    </div>
)

This worked for me, but I feel it is a bit hacky. You'll have to make sure that only one box is >95% visible. That could run into problems on various displays... WDYT @jskladan ?

@lbrabec , @jskladan please review the current implementation.

Yep, that looks good! I think we are getting there :)

Could you please replace underscore with space in breadcrumbs Beginner_guide and Manual_testing.

• Add bottom margin to the active step to make it a bit more spatial

Margin are quite large, it is basically a step per view. i believe that partially visible previous step and partially visible next step would help in orientation, but I'm not sure what exactly Josef had in mind. @jskladan could you please comment on this?

• I don't think the 'Previous' button is necessary, Move the next button to the right, and replace it with an ✔ button, that is grey at the beginning, and changes color to green, once clicked (also moves to the next step, as the current 'Next' button does)
• Add a visual difference to the "completed" steps (e.g. greenish tint to the header)

The greenish tint is everywhere, even on active 'box', this should be fixed. You already have the information which box is active (you put that into the URL), and since you know which step is active, you know which steps are completed.

@manishakanyal Thanks, this is absolutely going in the right direction!
Since I have a feeling, the written descriptions may not be the best option here, I put together a (very crude, mind you) animation of what I have in mind for the actual behaviour.
I could not have been bothered to make hundreds of frames by hand though, so please imagine that the animation actually is smooth :)

https://rajcze.eu/owncloud/index.php/s/Tm3O7YGcwM1zDSP

Hope that the animation makes the description from https://pagure.io/fedora-qa/landingpage/pull-request/79#comment-152535 straightforward. Thanks!

The greenish tint is everywhere, even on active 'box', this should be fixed. You already have the information which box is active (you put that into the URL), and since you know which step is active, you know which steps are completed.

@lbrabec, So currently what's happening is-

Whenever the user is scrolling down, we are considering visiting tabs as complete and making it a tint green header.

Whenever the user clicks on the complete button, that specific card is considered complete, and the button becomes tint green.

This is the current approach that I went through.
What changes should I move with?

@manishakanyal Thanks, this is absolutely going in the right direction!
Since I have a feeling, the written descriptions may not be the best option here, I put together a (very crude, mind you) animation of what I have in mind for the actual behaviour.
I could not have been bothered to make hundreds of frames by hand though, so please imagine that the animation actually is smooth :)

https://rajcze.eu/owncloud/index.php/s/Tm3O7YGcwM1zDSP

Hope that the animation makes the description from https://pagure.io/fedora-qa/landingpage/pull-request/79#comment-152535 straightforward. Thanks!

I got the animation, let me give it a try. Any leads for me though?

@jskladan, @lbrabec I'm having trouble implementing this animation can I get some help?

The trick is to use position: sticky; and set top for the boxes and put them inside a wrapper div with some height (which will serve here the same purpose as margin between boxes). What this does is:

• when you scroll the page, the box tries to stick to the top while the wrapper div actually moves upwards
• when the box hits the bottom of the wrapper element it cannot stick anymore and is pushed upwards together with the wrapper

.sticky-container {
  height: 70vh;
}
.sticky {
  position: sticky;
  top: 2em;
}

<div class="sticky-container">
  <div class="box sticky">
    box content
  </div>
</div>
<div class="sticky-container">
  <div class="box sticky">
    box content
  </div>
</div>
...

@lbrabec, okay let me give it a try.

@lbrabec, @jskladan I have tried many ways but nothing is resolving the issue, on clicking the next button the scrollIntoView is not working. Can you please help me?

Now on clicking the next button the animation is not working.

Hm, I can't get scrollIntoView() to work inside div with overflow-y: scroll, don't wrap the boxes in such div for now, that can be solved later by navbar and breadcrumbs with fixed position.

Also position: sticky should be set only for the boxes, not the wrapping div. You'll also need to remove height: 100% from the .card, if the card is 100% height of wrapping element it cannot stick and will scroll normally with the wrapping element.

I suggest to use ref for every element that wraps sticky box, e.g.

// this should be more elegant than hardcoded array
const boxRefs = [
  useRef(null),
  useRef(null),
  useRef(null),
  useRef(null),
]
// pass the next ref to Tab (be aware of the fact that the last Tab will index
// out of array so the result will be undefined)
<Tab ... onNext={() => onNext(boxRefs[index+1])} />
// use that ref in your onNext function to scroll
ref.current.scrollIntoView({behavior: "smooth", block: "start", inline: "nearest"})

@lbrabec, @jskladan I have implemented the changes, please review.

@lbrabec, @jskladan I have implemented the changes please have a look

@lbrabec, @jskladan I have implemented the changes, please have a look

@lbrabec, @jskladan please have a look

@lbrabec, @jskladan I have implemented the changes, please have a look.

Great job!

Couple of nitpicks:

• I see you're using react-scroll, could you please add offset={-150} to <Link>, that way part of the previous box would be visible (I chose the number 150 arbitrarily, check what works the best)
• please add bottom padding to the next buttons so they are not touching the border of boxes (same distance as from right would be nice)

Let's hear feedback from @jskladan. After that I'll continue with code review.

Sure thing, I implement the changes :)

LGTM. In my dream outcome, once you click on the "✔️" button, the "finished" boxes would keep the green tint from now on forever, and the scrolling would only affect the text's opacity.

If that could be done without a major overhaul, it would make me a better day, if it's just a PITA to do, I don't care for it so much to argue for the value of the time being spent on it.

@jskladan Sure thing let's move to the next thing.

@manishakanyal so, what's the source of the PITA there? Would be great if you could describe the problems in some reasonable detail. THX!

@jskladan, I have implemented the changes, please have a look!

Cool, LGTM. That's it from me, I guess. If the code is fine with @lbrabec, then let's merge and go to the next one!

Great!
@jskladan, @lbrabec are we not going to display any data on it, will it remain as it is?

We still have to write the texts, it will remain as is for now.

I'll proceed to code review now.

Manually writing ids (own, next and previous) is prone to errors.

Ideally the data structure is only [{title, description}] and rest is created automatically.
Plase, create a function that takes data without any id and returns data with ids:

[{title, description}] -> [{title, description, id, nextBox, prevBox}]

This shouldn't be hard coded, please create the object (the default state) dynamically based on processed data (see comment above).

Same here.

On the first step of wizard, please change the order of cards so "few hours" is on the left and "several days" is on the right, it makes more sense this way.

Are these three dependencies still needed?

@jskladan Do you think plain text is sufficient for text in boxes? Or do you think we'll need support for some text formatting (HTML tags are now rendered as is in text form)?

We still have to write the texts, it will remain as is for now.

I'll proceed to code review now.

Okay cool!

Manually writing ids (own, next and previous) is prone to errors.

Ideally the data structure is only [{title, description}] and rest is created automatically.
Plase, create a function that takes data without any id and returns data with ids:

[{title, description}] -> [{title, description, id, nextBox, prevBox}]

hmm, sure thing I'll create a function that takes data and returns data with ids.

Are these three dependencies still needed?

Nope not needed now

@lbrabec, I have implemented the changes, please have a look.

Code looks OK.

Please remove all warnings, rebase and squash commits to one. I'll then merge this PR.

There are still some warnings, please remove them.

@lbrabec, Removed warnings, please have a look

I still see some:

Compiled with warnings.
./src/wizard/manualTesting/Tab.js
  Line 20:6:  React Hook useEffect has missing dependencies: 'id' and 'props'. Either include them or remove the dependency array. However, 'props' will change when *any* prop changes, so the preferred fix is to destructure the 'props' object outside of the useEffect call and refer to those specific props inside useEffect  react-hooks/exhaustive-deps
./src/wizard/Manual.js
  Line 52:16:  'setTabs' is assigned a value but never used  no-unused-vars
Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.

• destructure props including setVisOfID and add all required deps
• tabs don't have to be in state at all since you're not using setTabs.

I destructured it, and the warning is still showing, other than visible I don't think so any other dependencies should be included.

Including id's and other deps will show these warnings

index.js:1 Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
    in Tab (at Manual.js:96)
    in div (at Manual.js:95)
    in Manual (at Step4.js:185)
    in div (created by Col)
    in Col (at Step4.js:184)
    in div (created by Row)
    in Row (at Step4.js:158)
    in Step4 (at WizardStepper.js:20)
    in div (at WizardStepper.js:16)
    in div (created by Container)
    in Container (at WizardStepper.js:15)
    in WizardStepper (at Wizard.js:33)
    in div (at Wizard.js:31)
    in div (created by Row)
    in Row (at Wizard.js:30)
    in div (created by Container)
    in Container (at Wizard.js:29)
    in div (at Layout.js:10)
    in Layout (at Wizard.js:28)
    in Wizard (at App.js:25)
    in Route (at App.js:22)
    in Switch (at App.js:13)
    in Router (created by BrowserRouter)

Oh, that's because setVisOfID is not always the same object in memory when component Manual re-renders [1] and since it is not the same object, the dependency changed and the effect is executed again (causing another rerender and so on...).

You can solve this by using useCallback without deps (https://reactjs.org/docs/hooks-reference.html#usecallback) and use it in the useEffect in Tab
or
don't create setVisOfID in Manual at all and pass down the state setter itself and use it in the useEffect this way: setVisibility(oldVis => ({...oldVis, [id]: visible}))

[1] see "Cause 3": https://typeofnan.dev/fix-the-maximum-update-depth-exceeded-error-in-react/

Removed warning @lbrabec, please have a look!

Great, thanks!

Merging.