Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(frontend): intempestive re-render of form #1019

Merged
merged 3 commits into from
Jul 10, 2019
Merged

fix(frontend): intempestive re-render of form #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e73870f
fix(frontend): intempestive re-render of form
UnbearableBear Jul 10, 2019
8f70099
fix(frontend): wrong proptypes
UnbearableBear Jul 10, 2019
da4baec
fix(frontend): bad practice in reducer
UnbearableBear Jul 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions packages/code-du-travail-frontend/src/outils/IndemniteLicenciement/Wizard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState } from "react";
import React, { useState, useReducer } from "react";
import PropTypes from "prop-types";
import styled from "styled-components";
import { Form } from "react-final-form";
Expand All @@ -9,14 +9,14 @@ import { StepItems } from "./StepItems";
import { PrevNextBar } from "./PrevNextBar";

function Wizard({
initialValues = {},
initialSteps,
initialStepIndex = 0,
steps,
onSubmit,
onUpdate,
rules = null
initialValues = {},
Rules = null,
stepReducer
}) {
const [stepIndex, setStepIndex] = useState(initialStepIndex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge this state part into the reducer ?

Copy link
Contributor Author

@UnbearableBear UnbearableBear Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so because the stepIndex is something internal to the wizard. So asking to the consumer of wizard to code it in the reducer would mean it could also impact the way pagination is handled. But it should not except for the initial step index. The only thing index can configure is the steps displayed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact I would leave the stepReducer up to the user but include it in a "WizzardReducer" wich handle the setIndex something like

case steps: return {...state, steps: stepReducer(state.steps, action)}

since reducers can be combined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I added this to the issue aswell, I will get back at it as soon as the planning gets more lightweight.

const [steps, dispatch] = useReducer(stepReducer, initialSteps);

const prevStep = () => {
setStepIndex(Math.max(0, stepIndex - 1));
Expand All @@ -36,12 +36,13 @@ function Wizard({
const handlePageSubmit = (values, form) => {
// This means the user clicked on a "restart a new simulation" button
if (stepIndex === steps.length - 1) {
onSubmit(values);
form.reset();
dispatch({
type: "reset"
});
setStepIndex(0);
} else {
nextStep();
onUpdate && onUpdate(values);
}
};
const stepItems = steps.map(({ name, label }) => ({
Expand Down Expand Up @@ -71,7 +72,9 @@ function Wizard({
return (
<>
<form onSubmit={handleSubmit}>
{rules}
{Rules && (
<Rules values={form.getState().values} dispatch={dispatch} />
)}
<StepItems activeIndex={stepIndex} items={stepItems} />
<StepWrapper narrow noPadding>
<Step form={form} />
Expand All @@ -92,10 +95,17 @@ function Wizard({
}

Wizard.propTypes = {
steps: PropTypes.array.isRequired,
initialValues: PropTypes.object,
stepReducer: PropTypes.func.isRequired,
initialSteps: PropTypes.arrayOf(
PropTypes.shape({
component: PropTypes.func.isRequired,
name: PropTypes.string.isRequired,
label: PropTypes.string.isRequired
})
).isRequired,
initialStepIndex: PropTypes.number,
onSubmit: PropTypes.func.isRequired
initialValues: PropTypes.object,
Rules: PropTypes.func
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PropTypes.node should it works for func and element)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to dislike it. Maybe it is because it is a basic function component ?
Screenshot 2019-07-10 at 15 48 45

};

export { Wizard };
Expand Down
72 changes: 37 additions & 35 deletions packages/code-du-travail-frontend/src/outils/IndemniteLicenciement/__tests__/Wizard.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from "react";
import { render } from "react-testing-library";
import { Wizard } from "../Wizard";
import { stepReducer } from "../stepReducer";
import { Field } from "react-final-form";

const FirstStep = () => <p>Premiere Etape</p>;
Expand All @@ -11,13 +12,6 @@ const SecondStep = () => (
Deuxieme Etape <button>Submit</button>
</p>
);
const FormStep = () => <Field name="firstName" component="input" />;

const FinalFormStep = {
label: "final form",
name: "final-form",
component: FormStep
};
const steps = [
{
name: "firstStep",
Expand All @@ -30,68 +24,76 @@ const steps = [
component: SecondStep
}
];

const AdditionalStep = () => <Field name="firstName" component="input" />;
const additionalStep = {
label: "Name",
name: "additional step",
component: AdditionalStep
};

describe("<Wizard />", () => {
it("should render a step", () => {
const onSubmit = jest.fn();
const { container } = render(<Wizard steps={steps} onSubmit={onSubmit} />);
expect(container).toMatchSnapshot();
});
it("should call onSubmit once finish", () => {
const onSubmit = jest.fn();
const { getByText } = render(
<Wizard steps={steps} onSubmit={onSubmit} initialStepIndex={1} />
const { container } = render(
<Wizard stepReducer={stepReducer} initialSteps={steps} />
);
const button = getByText(/submit/i);
button.click();
expect(onSubmit).toHaveBeenCalled();
expect(container).toMatchSnapshot();
});
it("should call navigate to the second step when click on Suivant", () => {
const onSubmit = jest.fn();
const onUpdate = jest.fn();
it("should navigate to the second step when click on Suivant", () => {
const { container, getByText } = render(
<Wizard steps={steps} onSubmit={onSubmit} onUpdate={onUpdate} />
<Wizard stepReducer={stepReducer} initialSteps={steps} />
);
const button = getByText(/suivant/i);
button.click();
expect(onUpdate).toHaveBeenCalled();
expect(container).toMatchSnapshot();
});
it("should call Step.validate when click on Suivant", () => {
const onSubmit = jest.fn();
const { getByText } = render(<Wizard steps={steps} onSubmit={onSubmit} />);
const { getByText } = render(
<Wizard stepReducer={stepReducer} initialSteps={steps} />
);
const button = getByText(/suivant/i);
button.click();
expect(FirstStep.validate).toHaveBeenCalled();
});
it("should handle initialStepIndex", () => {
const { container } = render(
<Wizard
stepReducer={stepReducer}
initialSteps={steps}
initialStepIndex={1}
/>
);
expect(container).toMatchSnapshot();
});
it("should call navigate the previous step when click on précédent", () => {
const onSubmit = jest.fn();
const { container, getByText } = render(
<Wizard steps={steps} onSubmit={onSubmit} initialStepIndex={1} />
<Wizard
stepReducer={stepReducer}
initialSteps={steps}
initialStepIndex={1}
/>
);
const button = getByText(/précédent/i);
button.click();
expect(container).toMatchSnapshot();
});
it("should handle initialValues", () => {
const onSubmit = jest.fn();
const { container } = render(
<Wizard
steps={[FinalFormStep]}
onSubmit={onSubmit}
stepReducer={stepReducer}
initialSteps={[...steps, additionalStep]}
initialValues={{ firstName: "lionel" }}
/>
);
expect(container).toMatchSnapshot();
});
it("should inject rules component", () => {
const onSubmit = jest.fn();
const Rule = () => <p>Rule</p>;
const { container } = render(
<Wizard
steps={[FinalFormStep]}
onSubmit={onSubmit}
initialValues={{ firstName: "lionel" }}
rules={[<Rule key="key" />]}
stepReducer={stepReducer}
initialSteps={steps}
Rules={() => <Rule key="key" />}
/>
);
expect(container).toMatchSnapshot();
Expand Down
Loading