OBPIH-6395 Add reusable useWizard hook by kchelstowski · Pull Request #4642 · openboxes/openboxes

kchelstowski · 2024-05-28T13:50:25Z

…useWizard hook

alannadolny · 2024-05-28T13:51:53Z

src/js/tests/hooks/useWizard.test.jsx

Great work on tests!

alannadolny · 2024-05-28T13:54:36Z

src/js/components/wizard/v2/WizardStepsV2.jsx

+  const isCurrentStep = (iteratedStep) => iteratedStep.key === currentStepKey;
+  return (
+    <div className="steps-main-box">
+      <div className="steps-inside-wrapper">


this className sounds slightly different to me: Can it be just "steps" or "steps-container"?

Those are the class names used in the current WizardSteps.jsx. I think we want to keep the same styling for now, so the only difference in this component and the current WizardSteps is that I eventually call the step.title at the end, not the step. I didn't want to do some workarounds in the existing WizardSteps, this is why I created a brand new compoment, as it will be more maintainable if we decide to change anything there and not break the existing one.

alannadolny · 2024-05-28T13:56:27Z

src/js/hooks/useWizard.js

+  };
+
+  const next = () => {
+    const nextStepIdx = steps.findIndex((s) => s.key === key) + 1;


Maybe it would be better to store the index of the key instead of recalculating the ID here and in the next 2 following functions?

To be honest I was doing that, but when I saw the complexity (or rather the readability) after such refactor, I decided that it would be better to do it this way, because otherwise, I would have to return both: step and index from the const Step, and imho this would be ugly to access the step then via, e.g. Step.step, if in the useMemo we returned:

return { step: foundStep, currentStepIdx: idx }

if you guys strongly feel it's worth, then I can do that again.

I also think I would prefer Alan's suggestion.

… wrapper components

alannadolny · 2024-05-29T08:31:42Z

src/js/tests/hooks/useWizard.test.jsx

+describe('Changing steps', () => {
+  const steps = [
+    {
+      key: 1,
+      Component: () => (<div>First compoment</div>),
+      title: 'First',
+    },
+    {
+      key: 2,
+      Component: () => (<div>Second compoment</div>),
+      title: 'Second',
+    },
+    {
+      key: 3,
+      Component: () => (<div>Third compoment</div>),
+      title: 'Third',
+    },
+    {
+      key: 4,
+      Component: () => (<div>Fourth compoment</div>),
+      title: 'Fourth',
+    },
+  ];
+
+  it('wizard will start with the initial key', () => {
+    const { result } = renderHook(() => useWizard({ initialKey: 3, steps }));
+
+    expect(result.current[0].key).toEqual(3);
+  });
+
+  it('wizard goes to next step', () => {
+    const { result } = renderHook(() => useWizard({ initialKey: 2, steps }));
+
+    act(() => {
+      result.current[1].next();
+    });
+
+    expect(result.current[0].key).toEqual(3);
+  });
+
+  it('wizard goes to previous step', () => {
+    const { result } = renderHook(() => useWizard({ initialKey: 4, steps }));
+
+    act(() => {
+      result.current[1].previous();
+    });


one nitpicky thing - the naming of tests (look into this comment) #3701 (comment)

Where is that convention documented? I can't find any resource opting for the "should" convention.

do we have any conventions documented? 😆 😆 😆 😆

When Alan was implementing tests a while ago, this was a request from me, "It should..." is a very common pattern for test name that explains the behavior and expected result like
"should return initial key"
"should advance to the next step when next is called"
"should go to the previous step when previous is called'"
"should go to the specific step when set is called"

alannadolny · 2024-05-29T08:51:56Z

src/js/components/stock-movement-wizard/fullOutbound/FullOutboundImport.jsx

+  }));
+
+  const [
+    Step,


Can you get Component and key by destructuring?
I would also like to make the object returned from the hook flat. It doesn't make sense to add an additional object.

return [ Step, set, first, next, previous, last, ];

imho it's cleaner to distinguish the step and the util hook functions, this is why I made it like this.

awalkowiak · 2024-05-29T10:22:05Z

grails-app/conf/runtime.groovy

                    defaultLabel: "Stock Movement",
                    menuItems: [
                        [label: "outbound.create.label", defaultLabel: "Create Outbound Movements", href: "/stockMovement/createOutbound?direction=OUTBOUND"],
+                        [label: "outbound.fullOutbound.import.label", defaultLabel: "Import Completed Outbound", href: "/stockMovement/fullOutboundImport"],


Perhaps it could be a simple: outbound.import.label?

awalkowiak · 2024-05-29T10:23:13Z

grails-app/controllers/org/pih/warehouse/inventory/StockMovementController.groovy

        render(view: "/common/react", params: params)
    }

+    def fullOutboundImport() {


Do we have to have a "full" keyword everywhere?

Yeah I was going to say we really should keep things simple. Try not to use PIH terminology if there's a way to make it simple. Imagine some old Greek guys having a conversation and then distill their response.

Socrates: What does it do?
Kacperian: It provides a way to import a stock movement including all of its dependent objects like requests, picklists, etc.

✨

importStockMovement()

✨

or

importOutboundStockMovement()

I would go with the 2nd option, especially if we are supposed to distinguish the SMs in the future for inbound, outbound etc.

awalkowiak · 2024-05-29T10:25:46Z

src/js/components/wizard/v2/WizardStepsV2.jsx

+
+import 'components/wizard/WizardSteps.scss';
+
+const WizardStepsV2 = ({ steps, currentStepKey }) => {


What has changed here compared to WizardSteps?

I answered that question in one of the comments below: https://github.com/openboxes/openboxes/pull/4642/files#r1617300455

awalkowiak · 2024-05-29T10:41:51Z

src/js/components/stock-movement-wizard/fullOutbound/FullOutboundImport.jsx

+import useWizard from 'hooks/useWizard';
+
+const FullOutboundImport = () => {
+  const FullOutboundImportStep = {


FullOutboundImportSteps

awalkowiak · 2024-05-29T10:42:33Z

src/js/components/stock-movement-wizard/fullOutbound/FullOutboundImport.jsx

+      title: 'Confirm',
+    },
+  };
+  const steps = useMemo(() => [


This feels a bit weird. Can we just have an object with full steps structure that would be transformed to list of values? Feels like this part could be simplified

awalkowiak · 2024-05-29T10:43:46Z

src/js/hooks/useWizard.js

+  };
+
+  const next = () => {
+    const nextStepIdx = steps.findIndex((s) => s.key === key) + 1;


I also think I would prefer Alan's suggestion.

drodzewicz · 2024-05-29T12:56:22Z

src/js/components/wizard/v2/WizardStepsV2.jsx

+          <div
+            key={step.key}
+            className={`step-container ${isCurrentStep(step) ? 'active' : ''}`}
+            data-testid={isCurrentStep(step) ? 'active' : 'inactive'}


data-testid should not change conditionally I believe this should data-testid="wizzard-step" and if you need to specify the state of the step we should use a different attribute like a custom
data-stepstate={isCurrentStep(step) ? 'active' : 'inactive'}
or
aria-label={isCurrentStep(step) ? 'active' : 'inactive'}

It was copied from the existing WizardSteps, so I did not even realize and thought about it, but since you are testing ninja, I trust you 😆

drodzewicz · 2024-05-29T12:58:55Z

src/js/hooks/useWizard.js

+  const set = (val) => {
+    setKey(val);
+  };


this looks unnecessary we can just return the setKey function in the hook like

return { set: setKey }

drodzewicz · 2024-05-29T13:01:39Z

src/js/hooks/useWizard.js

+  const first = () => {
+    setKey(steps[0]?.key);
+  };
+
+  const last = () => {
+    const lastIdx = steps.length - 1;
+    setKey(steps[lastIdx]?.key);
+  };


It's cool that you added these but are we ever going to use them?
I don't care if they stay or they go, I am just not familiar with any use case where we will want to jump to first and last steps.

We already have such use cases - e.g. if we have an SM in progress and it is supposed to be on "X" step, we go to step "X".

yes, but I am not talking about function set but last and first

maybe now we don't have the use case, but they felt cute and I also wanted to have a complete hook, that would cover any of the future cases, so I kept them :sadsmile:
Let me know if I should remove them in your opinion guys.

drodzewicz · 2024-05-29T13:14:22Z

src/js/tests/hooks/useWizard.test.jsx

+describe('Changing steps', () => {
+  const steps = [
+    {
+      key: 1,
+      Component: () => (<div>First compoment</div>),
+      title: 'First',
+    },
+    {
+      key: 2,
+      Component: () => (<div>Second compoment</div>),
+      title: 'Second',
+    },
+    {
+      key: 3,
+      Component: () => (<div>Third compoment</div>),
+      title: 'Third',
+    },
+    {
+      key: 4,
+      Component: () => (<div>Fourth compoment</div>),
+      title: 'Fourth',
+    },
+  ];
+
+  it('wizard will start with the initial key', () => {
+    const { result } = renderHook(() => useWizard({ initialKey: 3, steps }));
+
+    expect(result.current[0].key).toEqual(3);
+  });
+
+  it('wizard goes to next step', () => {
+    const { result } = renderHook(() => useWizard({ initialKey: 2, steps }));
+
+    act(() => {
+      result.current[1].next();
+    });
+
+    expect(result.current[0].key).toEqual(3);
+  });
+
+  it('wizard goes to previous step', () => {
+    const { result } = renderHook(() => useWizard({ initialKey: 4, steps }));
+
+    act(() => {
+      result.current[1].previous();
+    });


When Alan was implementing tests a while ago, this was a request from me, "It should..." is a very common pattern for test name that explains the behavior and expected result like
"should return initial key"
"should advance to the next step when next is called"
"should go to the previous step when previous is called'"
"should go to the specific step when set is called"

jmiranda

@kchelstowski Wait does this mean I can finally click on the wizard steps to go through the flow? I'm so confused. And excited.

kchelstowski · 2024-05-29T13:42:53Z

@jmiranda yes, I've just done the "background" for such behavior, but to be honest we've already had such a behavior, but it was kinda "freezed" (it's been prepared but it has been disabled throughout all of the current wizards)

awalkowiak · 2024-06-03T14:02:45Z

@jmiranda clicking on a step is supported, but disabled/not used on "old" wizards.

kchelstowski added 4 commits May 28, 2024 15:47

OBPIH-6395 Add react-hooks testing library

19808f1

OBPIH-6395 Add WizardStepsV2 component to be integrated with the new …

65d7485

…useWizard hook

OBPIH-6395 Add useWizard hook

d854d21

OBPIH-6395 Add unit tests for useWizard hook

1be7055

kchelstowski requested review from alannadolny, awalkowiak, drodzewicz and ewaterman May 28, 2024 13:50

kchelstowski self-assigned this May 28, 2024

alannadolny reviewed May 28, 2024

View reviewed changes

kchelstowski added the warn: do not merge Marks a pull request that is not yet ready to be merged label May 29, 2024

OBPIH-6395 Add full outbound import feature to the Menu and add empty…

e3b97d4

… wrapper components

kchelstowski removed the warn: do not merge Marks a pull request that is not yet ready to be merged label May 29, 2024

alannadolny reviewed May 29, 2024

View reviewed changes

awalkowiak reviewed May 29, 2024

View reviewed changes

drodzewicz requested changes May 29, 2024

View reviewed changes

jmiranda reviewed May 29, 2024

View reviewed changes

OBPIH-6395 Fixes after review

ffdfb68

kchelstowski requested review from alannadolny, awalkowiak and drodzewicz June 3, 2024 08:53

drodzewicz approved these changes Jun 3, 2024

View reviewed changes

awalkowiak merged commit 8f81281 into develop Jun 3, 2024

awalkowiak deleted the OBPIH-6395 branch June 3, 2024 14:02

+                }));
+                const [
+                  Step,


		import 'components/wizard/WizardSteps.scss';

		const WizardStepsV2 = ({ steps, currentStepKey }) => {

Uh oh!

Conversation

kchelstowski commented May 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchelstowski May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drodzewicz May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmiranda left a comment

Choose a reason for hiding this comment

Uh oh!

kchelstowski commented May 29, 2024

Uh oh!

awalkowiak commented Jun 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kchelstowski May 29, 2024 •

edited

Loading

drodzewicz May 29, 2024 •

edited

Loading