Skip to content

Feature/checkbox form#2

Open
crisanlucid wants to merge 8 commits into
developfrom
feature/checkbox-form
Open

Feature/checkbox form#2
crisanlucid wants to merge 8 commits into
developfrom
feature/checkbox-form

Conversation

@crisanlucid
Copy link
Copy Markdown
Owner

No description provided.

@booya2nd
Copy link
Copy Markdown

General question did you intentionally avoided a redux pattern (action->reducer->store) ?

...formState
};

for (let key in updateFormPage) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

errrm ... should not for..in in general imho

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

sure, I am trying to keep it simple, which means I have only Parent Business Login and trying to propagate the methods to the Functional Component

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

updateFormPage is object not array

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think Object.entries would be your better choice here

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I can replace for ... in with Object.entries()

};

for (let key in updateFormPage) {
if ('text' !== updateFormPage[key]['elementConfig']['type']) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this looks structurally intimidating at first glance ...

if ('text' !== updateFormPage[key]['elementConfig']['type']) {
updateFormPage[key]['value'] = updateFormPage[
key
].elementConfig.options.reduce((acc, { label, value }) => {
if (value === updateFormPage[key].elementConfig.defaultOption) {
acc = label;
}
return acc;
}, '');
}

can you try to write the code that the reader is able to understand what it does without parsing each line ?

};
updatedFormElement.value = event.target.value;

if ('radio' === updatedFormElement.elementConfig.type) {
Copy link
Copy Markdown

@booya2nd booya2nd Mar 14, 2019

Choose a reason for hiding this comment

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

also this is one more "wtf" block - more readybility in terms of getting WHAT / WHAT-FOR it does - this way I can read it line by line in terms of HOW

if ('radio' === updatedFormElement.elementConfig.type) {
updatedFormElement.elementConfig.defaultOption =
event.currentTarget.value;
updatedFormElement.value = updatedFormElement.elementConfig.options.reduce(
(acc, { label, value }) => {
if (value === event.currentTarget.value) {
acc = label;
}
return acc;
},
''
);
} else {
updatedFormElement.value = event.target.value;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I can extract and put into a arrow function to be more readable

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes adopting clean-code principals and moving logic into named functional scopes will help a lot here :)

const panelType = quizForm[formState.step].typePanel || '';
let panelType = quizForm[formState.step].typePanel || '';

const calcAnswersTOTAL = updatedForm => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would try to get this information from the store, as you already have to update the state per answered question, it would be easy to have the value stored there

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I found it, from performance point of view to have a function to calculate all the answered questions and I called once.

Form.js is a Functional Component and is does not have proper State

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In general "performance-reasons" or rather performance optimizations should be tackled when measurable / testable issues are discovered in an early version - as long as there is no issue I would always follow the best approach for maintainability and or readability.

let primaryMessage = 'Game Over';
const points = calcAnswersTOTAL(quizForm);

if (points >= 6) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd say avoid magic numbers - read it from a config / named-constant

onChange={props.changed}
/>
);
if ('radio' === props.elementConfig.type) {
Copy link
Copy Markdown

@booya2nd booya2nd Mar 14, 2019

Choose a reason for hiding this comment

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

general pattern of special/specific code in generalized component does not feel right:

if ('radio' === props.elementConfig.type) {
inputElement = props.elementConfig.options.map((option, index) => {
return (
<div
className={inputClasses.join(' ')}
key={`${props.elementConfig.name}-${index}`}
>
<label>
<input
type={props.elementConfig.type}
name={props.elementConfig.name}
value={option.value}
checked={props.selectedOption === option.value}
onChange={props.changed}
className="form-check-radio"
/>
{option.label}
</label>
</div>
);
});
} else {
inputElement = (
<input
className={inputClasses.join(' ')}
{...props.elementConfig}
value={props.value}
onChange={props.changed}
/>
);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants