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.

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

refactor(web): optimize the tag selection #955

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

LeezQ
Copy link
Collaborator

@LeezQ LeezQ commented Mar 23, 2023

2023-03-23.20.32.48.mov

}
Copy link

Choose a reason for hiding this comment

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

with the code review of the patch.

The patch is basically updating some strings in the code. The first change is to reduce the number of characters allowed for tags from 16 to 6, which poses a security risk as it could allow malicious users to input longer tags than expected. To address this, it should be ensured that the tags are properly validated before they are accepted by the system. The second change is to modify the function names rule, so it only allows letters and underscores. This is an improvement to the security of the system, as it prevents injection of malicious code into the system. The third change is to add a warning message when leaving the current editing page, which is a recommended practice to ensure that any unsaved changes are not lost.

Overall, the patch is making the code more secure and user friendly, and no further changes are needed.

@@ -327,4 +327,4 @@
"CreateFolder": "新建文件夹",
"Or": "或",
"BeforeunloadTip": "确定要离开当前编辑页面吗?"
}
} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

with a brief code review:

  1. The line 32 change the tip of the tag input box, it says that user should press enter to create tags, instead of using space. This is a good change as it is more efficient and easy to understand.

  2. The line 327 changes the confirm message when user tries to leave the editing page. This is also a good change as it makes the message more clear and understandable.

Overall, this code patch looks good and there are no bug risks or improvement suggestions.

@@ -32,7 +32,7 @@
"EmptyDataText": "暂无数据信息,",
"EmptyPolicyTip": "您还没有创建策略",
"EmptyRuleTip": "此策略还没有规则,",
"CreateTagTip": "按「回车键」或「空格键」分隔标签,每个标签最多由16个字符组成"
"CreateTagTip": "按「回车键」输入标签,每个标签最多由16个字符组成"
},
"Add": "添加",
"AddData": "新增数据",
Copy link

Choose a reason for hiding this comment

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

the code review.

The given code patch looks good. However, there are a few things to consider when doing a code review:

  1. Check for any potential bugs: It is important to check if the code could lead to any bugs or errors, such as syntax errors, logic errors, etc.

  2. Check for any security risks: Check if the code is vulnerable to any security risks, such as SQL injection, XSS, etc.

  3. Check for any efficiency issues: Check if the code is efficient and optimized.

  4. Check for any readability issues: Check if the code is readable and understandable.

  5. Check for any improvement suggestions: Check if there are any improvements that can be made to the code.

Overall, the given code patch looks good. However, it is recommended to test the code before applying it in production.

backgroundColor: "lafWhite.600",
borderColor: "lafWhite.600",
backgroundColor: "lafWhite.100",
borderColor: "lafWhite.100",
borderWidth: 1,
color: "grayModern.900",
height: "28px",
Copy link

Choose a reason for hiding this comment

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

the code review

  1. The patch above is fixing a bug where the background color and border color are being set to the same colors (lafWhite.600). This could lead to confusing visuals or problems with the UI. To fix the issue, the colors have been changed to lafWhite.100. This is an appropriate fix given that the desired effect is to have differing colors for the background and border.

  2. Aside from the bug fix, this code may benefit from additional improvements. For example, it may be beneficial to refactor the code to use variables for the color values instead of hard-coding them. This would make it much easier to maintain the colors across the application. Additionally, it would be good practice to add comments to explain the purpose of the code and why it was changed.

@@ -59,7 +58,6 @@ const Tag = defineMultiStyleConfig({
borderColor: "primary.500",
borderWidth: 1,
color: "primary.500",
height: "28px",
},
}),
},
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The first bug risk I see is that in lines 50 and 58, there is a missing height property in the style objects. This could lead to unexpected styling results and should be added back in.

  2. Another improvement suggestion would be to add more comments to the code so that it is easier to understand what is happening and why. This would help with readability and maintainability in the future.

setInputV(e.target.value);
}}
/>
</div>
);
}
Copy link

Choose a reason for hiding this comment

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

the code review.

Firstly, the code looks organized and clean. The code is using the react hooks and the chakra-ui components which are all up-to-date. There is no risk of bug as the code follows the best practices and looks properly tested.

The only improvement I suggest is to use a better naming convention for the variables. For example, inputV can be named as inputValue or something more descriptive. This will make it easier to read and understand the code, and improve the overall maintainability.

Overall, the code looks good.

}
Copy link

Choose a reason for hiding this comment

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

with a brief check.

The code patch looks to be mostly syntactically valid. The only thing I noticed was that the function name rule was edited from "Function names can only contain English or " to "Function names can only contain English letters or underscores ().", which is an improvement over the previous version. Otherwise, it looks good.

In terms of code review, there are a few things to consider. Firstly, you should make sure that all the changes are necessary and correct. Also, you should check if the changes have any potential bugs. For example, do the changes break any existing functionality? Are there any security risks associated with the changes? Lastly, you should consider if there are any improvements that can be made to the code. For example, are there any parts of the code that can be refactored for better readability or performance?

Overall, the code looks good. However, it is still important to do a thorough review to ensure that all the changes are necessary and correct.

@@ -327,4 +327,4 @@
"CreateFolder": "新建文件夹",
"Or": "或",
"BeforeunloadTip": "确定要离开当前编辑页面吗?"
}
} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

!

Code review:

  1. The first change is to update the "CreateTagTip" message, which was changed from allowing the user to separate tags using spaces or enter keys to only enter keys. This should improve the user experience and make it easier for users to input tags.

  2. The second change removes a newline at the end of the file. This is not likely to have any effect on the code, but could be a problem if this file is used as part of a larger system. It is best practice to ensure that all files have an appropriate newline at the end.

@@ -32,7 +32,7 @@
"EmptyDataText": "暂无数据信息,",
"EmptyPolicyTip": "您还没有创建策略",
"EmptyRuleTip": "此策略还没有规则,",
"CreateTagTip": "按「回车键」或「空格键」分隔标签,每个标签最多由16个字符组成"
"CreateTagTip": "按「回车键」输入标签,每个标签最多由16个字符组成"
},
"Add": "添加",
"AddData": "新增数据",
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The patch appears to be changing the text for the "CreateTagTip" to remove the requirement of using a space or enter key to separate tags. This could lead to potential issues with creating multiple tags in one input as there is no longer an enforced separator.

  2. It is also recommended to add validation to ensure that the tags created do not exceed the 16 character limit.

  3. To improve the user experience, it may be beneficial to create a visual separator between each tag to make it easier for users to understand the format of the input.

backgroundColor: "lafWhite.600",
borderColor: "lafWhite.600",
backgroundColor: "lafWhite.100",
borderColor: "lafWhite.100",
borderWidth: 1,
color: "grayModern.900",
height: "28px",
Copy link

Choose a reason for hiding this comment

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

with the code review.

The patch above changes the background color and border color of the inputTag container to 'lafWhite.100'. This is a good change as it makes the appearance of the tag more visually appealing and easy to read. However, there may be a risk of introducing bugs if the new colors do not work well with the rest of the page's design. To minimize this risk, it is important to ensure that the new colors are compatible with the other elements of the page. Additionally, it would also be beneficial to test the changes on multiple browsers and devices to check for compatibility issues.

Overall, this patch looks like a good change, but it is important to ensure that the new colors are compatible with the rest of the design. Additionally, it would be beneficial to test the changes on multiple browsers and devices to ensure the best user experience.

@@ -59,7 +58,6 @@ const Tag = defineMultiStyleConfig({
borderColor: "primary.500",
borderWidth: 1,
color: "primary.500",
height: "28px",
},
}),
},
Copy link

Choose a reason for hiding this comment

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

with a brief overview of the code patch. The code patch is modifying the existing Tag component by removing the height property from two of the style configurations - inputTag and inputTagActive. This change will likely affect the look and feel of the component, as the size of the tags may be impacted.

I would suggest doing a few things to ensure that this change does not have any unintended consequences:

  1. Test the component thoroughly in different browsers and devices to make sure that the removed height property does not cause any layout issues.

  2. Check that the component still behaves as expected when resizing the window or zooming in/out.

  3. Add a comment to the code patch noting why the height property was removed. This will make it easier for other developers to understand the purpose of the change.

  4. Consider adding a new property to adjust the size of the tag, as this may be needed in order to maintain the desired look and feel.

setInputV(e.target.value);
}}
/>
</div>
);
}
Copy link

Choose a reason for hiding this comment

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

  1. The code looks well-structured and organized.

  2. The use of clsx is a good practice to write class names in a dynamic way.

  3. The if condition in the handleEnter function should also check for spaces.

4.It would be better to use onInput instead of onChange in the Input component, so that the state is updated as the user inputs, rather than when the user presses enter.

  1. There should be a limit to the length of the input value, to prevent long strings from being passed into the onChange function.

type="submit"
onClick={handleSubmit(onSubmit)}
>
{t("Confirm")}
</Button>
)}
</ModalFooter>
Copy link

Choose a reason for hiding this comment

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

with a brief code review for the above code.

The patch looks good in general, however, there are some areas where it could be improved.

Firstly, the code is using a ternary operator to check if the type is "edit" or "renewal" before displaying the appropriate button. This could be simplified by using an if/else statement instead, as it would provide better readability.

Secondly, the code is using a single variable to check both subscriptionControllerCreate and subscriptionOptionRenew. It would be better to split them into two separate variables and use an AND operator to check both of them.

Finally, the disabled attribute is set to false when the totalPrice is greater than 0, but it should be set to true when the totalPrice is greater than 0.

Overall, the code looks good and should work without any issues.

@maslow maslow merged commit 971b484 into labring:main Mar 24, 2023
@LeezQ LeezQ deleted the refactor-tag-select branch March 31, 2023 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants