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

fix(comp:timeline): modify timeline style according to design #1509

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

sallerli1
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added/updated or not needed
  • Docs and demo have been added/updated or not needed

What is the current behavior?

时间线组件样式不符合设计规范

What is the new behavior?

根据seer规范修改组件样式

Other information

API没有变,组件样式有改变,dom结构有轻微变动,看是否需要改成feature

{slots.default && <div class={`${prefixCls}-content-desc`}>{slots.default()}</div>}
{labelNode && <div class={`${prefixCls}-content-label`}>{labelNode}</div>}
</div>
</li>
)
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 use of indentation and tabs should be consistent throughout the code.
  2. It is good practice to add comments to explain what each block of code does.
  3. The code should be optimized by removing unnecessary variables and functions.
  4. Avoid using global variables and instead use local variables.
  5. The code should be checked for any security vulnerabilities.
  6. The code should be tested with multiple browsers and devices to make sure that it works correctly.
  7. The code should be refactored to ensure that it is as efficient as possible.
  8. Code should be organized into functions for better readability.
  9. Use descriptive variable and function names to make the code more understandable.
  10. Unused variables should be removed from the code.

}
}

&-start {
.@{timeline-item-prefix} {
&-content {
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 introduction about the code
This code is used to customize the Timeline component. The code is written in Less (a CSS pre-processor) and it is defined by the variables, mixins and functions. The code adds the styling for timeline components, such as dot size, dot border, content label color, and so on.

Now, let's do a code review:

  1. There are some unnecessary lines in the code like line 8, 29, 92 and 103 which can be removed.
  2. In line 59, some margin-top value should be added to the &-label class.
  3. In line 119 and 136, there is a missing semicolon at the end of the line.
  4. In line 158, the &-not-both class is missing a semicolon at the end of the line.
  5. Some more classes and styling can be added to customize the timeline component further.

@idux-bot
Copy link

idux-bot bot commented Mar 23, 2023

This preview will be available after the AzureCI is passed.

@@ -25,7 +46,6 @@

.timeline-dotted-line() {
.@{timeline-item-prefix} {

&-head-line {
background: @timeline-dotted-line-background;
background-size: @timeline-dotted-line-background-size;
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

This code patch is written in SCSS and it is used to create classes for timeline-color and timeline-dot-color. It also uses Sass functions like length(), extract(), ~ (string interpolation) etc. The code patch also adds classes for timeline-item-prefix, timeline-dotted-line-background, and timeline-dotted-line-background-size.

Now let's do a code review.

First, I would check the indentation and formatting of the code to ensure that it is consistent and easy to read.

Second, I would review the logic of the code to make sure that it is valid and that there are no potential bugs. For example, I would review the use of the length() and extract() functions, as well as the if/else statements, to make sure that they are being used correctly.

Third, I would check for any potential performance issues. For example, I would check for any unnecessary loops or redundant code that could be optimized for better performance.

Finally, I would look for any potential security issues. For example, I would check for any potential SQL injection attacks or insecure data handling.

Overall, this code patch looks good and should not have any major issues. If you have any questions or concerns about the code, please let me know.

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #1509 (ec8eb3f) into main (703122f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ec8eb3f differs from pull request most recent head 4520ad6. Consider uploading reports for the commit 4520ad6 to get more accurate results

@@           Coverage Diff           @@
##             main    #1509   +/-   ##
=======================================
  Coverage   92.75%   92.75%           
=======================================
  Files         331      331           
  Lines       30801    30812   +11     
  Branches     3533     3537    +4     
=======================================
+ Hits        28568    28579   +11     
  Misses       2233     2233           
Impacted Files Coverage Δ
packages/components/timeline/src/TimelineItem.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

danranVm
danranVm previously approved these changes Mar 24, 2023
{slots.default && <div class={`${prefixCls}-content-desc`}>{slots.default()}</div>}
{labelNode && <div class={`${prefixCls}-content-label`}>{labelNode}</div>}
</div>
</li>
)
Copy link

Choose a reason for hiding this comment

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

with code review

  1. There is unnecessary import type from 'vue' which can be removed.
  2. In the eslint disabled code block, there is a comma after style={props.color} which should be removed.
  3. In the classes computed, the placement variable is not used, so it can be removed.
  4. The dotClass computed can be changed to use ternary operator instead of if-else statement.
  5. In the last line of the code, there is a

}
}

&-start {
.@{timeline-item-prefix} {
&-content {
Copy link

Choose a reason for hiding this comment

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

from the first line:

The code looks ok. There are no major bugs or risks that I can see. The only improvement suggestion I would make is to add more comments to the code to make it easier for others to understand.

@@ -25,7 +46,6 @@

.timeline-dotted-line() {
.@{timeline-item-prefix} {

&-head-line {
background: @timeline-dotted-line-background;
background-size: @timeline-dotted-line-background-size;
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.

First, I would suggest that you add comments to the code patch to explain what it does and why you are making changes. This will help others understand the code better and provide more context about your changes.

Next, I would suggest that you check for any potential bugs or errors in the code. You should also look for any redundant or unnecessary code that can be removed to improve performance. Additionally, you should consider the readability of the code, as well as its maintainability.

Finally, if there are any improvements that can be made to the code or any suggestions for alternative approaches, these should be noted so that they can be discussed further.

Overall, it is important to make sure that the code meets any standards or guidelines that have been established, such as coding conventions or formatting rules.

I hope this helps!

@@ -14,6 +14,7 @@
@timeline-content-gap: @timeline-item-timeline-width;
@timeline-content-color: @text-color;
@timeline-content-label-color: @color-graphite;
@timeline-content-label-margin-top: 0;

@timeline-line-gap: ((@timeline-dot-size - @timeline-dot-border-width) / 2);
@timeline-line-width: 2px;
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:

  1. The first line defines the gap between timeline items, which is the same as the timeline item width. This ensures that the timeline items are properly spaced out.

  2. The second line defines the color of the timeline content, which should be the same as the text color. This will ensure a consistent look and feel across the timeline.

  3. The third line defines the color of the timeline label, which should be set to Graphite. This will provide a good contrast against the other colors used in the timeline.

  4. The fourth line adds a margin-top value of 0 for the timeline label. This will help move the label away from the timeline items and make it easier to read.

Overall, the code looks good and should provide the desired functionality. There do not appear to be any bugs or risks associated with the code. However, if you wish to improve the code, it might be beneficial to add additional comments to explain the purpose of each line. This will help other developers who may be looking at the code in the future.

@@ -1 +1,4 @@
@import './default.variable.less';

@timeline-line-height: 1;
@timeline-content-label-margin-top: 8px; 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.

The given code patch looks correct and there is no bug risk. The code is importing a less file and setting timeline-line-height and timeline-content-label-margin-top variables.

As far as improvement suggestions, it depends on the context of the project. If you are looking to improve performance, you could look into caching the imported less file or using a different file type that is more performant. You might also want to consider adding a comment in the code explaining what these variables do, so that other developers can understand the purpose of the code.

@danranVm danranVm merged commit b9db622 into IDuxFE:main Mar 27, 2023
@sallerli1 sallerli1 deleted the fix-timeline-style branch July 4, 2024 09:27
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.

None yet

2 participants