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.
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
LPS-137025 Validate/Create fields rules to Business Info - Info Panel #123
LPS-137025 Validate/Create fields rules to Business Info - Info Panel #123
Conversation
jairmedeiros
commented
Aug 9, 2021
•
edited
edited
- LPS-137025 Create hook to get Legal Entities
- LPS-137025 Add new inputs to attend the mock
- LPS-137025 Add First Select option for non Default Values
- LPS-137025 Adjust placeholder to add the percentage
- LPS-137025 Fix defaultValue prop to Select and Switch
- LPS-137025 Add Default Values to ControlledSwitch
- LPS-137025 Fix Legal Entities list
- LPS-137025 Add new REGEX to validate percentage until 100
- LPS-137025 Export getCategoryProperties instead getBusinessClassCode
- LPS-137025 Save segment in form's value
- LPS-137025 Validate BCC and Segment to show some fields
- LPS-137025 Fix Step Percentage to Business field
- LPS-137025 Fix indentations
- LPS-137025 Centralize logic to businessFields
- LPS-137025 Remove default value for placeholder parameter in InputWithMask
- LPS-137025 Remove option hidden for Select atom
- LPS-137025 Pass pattern within rules prop
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
Just starting reviewing :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jairmedeiros my dear!
At first, congratulations for the PR, everything is amazing! 🎉
It's really, really good!
I did some considerations for your code, but, I would like to get some advices from @joseabelenda and @giu7d to merge this code.
But for me, it could be merged.
@@ -48,6 +48,7 @@ export const FormBusiness = () => { | |||
label="Do you store personally identifiable information about your customers?" | |||
rules={{ required: true }} | |||
control={control} | |||
defaultValue="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jairmedeiros , I noted that here defaultValue
is true, as string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevenleone this is required to prevent the DOM to think a boolean is been used as an attribute... which could lead to unexpected behavior.
I suggest that if we want to remove the "true"/"false" as strings to improve the code readability that we change the name... but still, it needs to be a string.
Also, the isValid
will think the field is not valid if it has the false as a boolean value. Maybe we should just change this to "yes" or "no" to avoid this problem at all...
@@ -80,6 +81,7 @@ export const FormBusiness = () => { | |||
label="Do you sell products under your own brand or label?" | |||
rules={{ required: true }} | |||
control={control} | |||
defaultValue={false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But here, defaultValue
is false as boolean.
That's the idea, or should everything be a string or boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a string...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put false
, because following the mock, this field doesn't have a default value. When a remove this parameter the field stay with "no" selected by default. How can I do it right? ty.
|
||
const businessTotalFields = (properties) => { | ||
let fieldCount = 4; | ||
const bccAllowedToField = ["750", "1349"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future, we can't assign this value as fixed, right @joseabelenda ?
Maybe a way to workaround this, should make another request for categorization api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a request that return the BCCs allowed for appear some fields? I was following the spreadsheet where some fields only appears when the product's BCC is 750 or 1349, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok @jairmedeiros , I think we don't have this for now
@@ -0,0 +1,21 @@ | |||
export const BCC_ALLOWED = { | |||
PERCENT_SALES: ["750", "1349"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now it's okay, but the same from previous: this should be dynamic in the future, isn't @joseabelenda ?
value: PERCENTAGE_REGEX, | ||
message: "Must be a valid percentage", | ||
}, | ||
pattern, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, a pattern will always be overwritten by a rule...
So if I do:
<PercentageControlledInput pattern={...something} rule={{ pattern: { ...something2 }}} />
The pattern={...something}
will never be used.
I suggest you keep the old structure to use a component that extends the react-hook-form semantic and prevents unnecessary props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -48,6 +48,7 @@ export const FormBusiness = () => { | |||
label="Do you store personally identifiable information about your customers?" | |||
rules={{ required: true }} | |||
control={control} | |||
defaultValue="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevenleone this is required to prevent the DOM to think a boolean is been used as an attribute... which could lead to unexpected behavior.
I suggest that if we want to remove the "true"/"false" as strings to improve the code readability that we change the name... but still, it needs to be a string.
Also, the isValid
will think the field is not valid if it has the false as a boolean value. Maybe we should just change this to "yes" or "no" to avoid this problem at all...
value: "salesMerchandise", | ||
}} | ||
control={control} | ||
pattern={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this pattern not to be a default rule of the PercentageControlledInput
?
Also, I think it should be used as part of the rules
prop to keep the react-hook-form semantic:
<PercentageControlledInput ... rules={{ pattern={ ...something } }} />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason haha. I thought to keep for reuse where to be required a percentage field with more than 100%. I will fix it to pass within the rules props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -12,6 +12,7 @@ export const InputWithMask = React.forwardRef( | |||
error, | |||
allowNegative = false, | |||
required = false, | |||
placeholder = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placeholder attribute was already handled by the ...props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, I will fix it too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -21,6 +21,7 @@ export const Select = React.forwardRef( | |||
className="input" | |||
required={required} | |||
> | |||
<option hidden>Select</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should not add this to the Select
atom.
The Select
fragment is an atomic component that represents a pure select input reusable by any component. Logic belongs outside the fragment
as a connector
or container
component.
I would move the <option hidden>Select</option>
were it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -80,6 +81,7 @@ export const FormBusiness = () => { | |||
label="Do you sell products under your own brand or label?" | |||
rules={{ required: true }} | |||
control={control} | |||
defaultValue={false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a string...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jairmedeiros , looking into @giu7d comments, can you do changes where is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @jairmedeiros! I think it is good to go 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 🔥
Thank you guys, @giu7d , @jairmedeiros
Just starting reviewing :) |
resent: #131 |