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

Jump to bottom

fix(comp:slider): slider marker color error when disabled #1457

Merged
merged 1 commit into from
Feb 23, 2023
Merged

fix(comp:slider): slider marker color error when disabled #1457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions packages/components/slider/demo/Marks.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
<template>
<h4>marks & step</h4>
<IxSlider v-model:value="value0" :marks="marks"></IxSlider>
<IxSlider v-model:value="value0" :disabled="isDisabled" :marks="marks"></IxSlider>

<h4>step=null</h4>
<IxSlider v-model:value="value1" :marks="marks" :step="null"></IxSlider>
<IxSlider v-model:value="value1" :disabled="isDisabled" :marks="marks" :step="null"></IxSlider>

Disabled: <IxSwitch v-model:checked="isDisabled"></IxSwitch>
</template>

<script setup lang="ts">
Expand All @@ -23,4 +25,5 @@ const marks = {

const value0 = ref(30)
const value1 = ref(50)
const isDisabled = ref(false)
</script>
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. In the template section, it looks like a new attribute "disabled" has been added to the IxSlider components. This is a good addition since it will allow users to control whether or not the IxSlider should be disabled.

  2. In the script section, it looks like a new variable has been added called "isDisabled". This variable is used to control the disabled state of the IxSlider. This is a good addition since it allows the user to easily control the disabled state of the IxSlider.

  3. It looks like an IxSwitch component has also been added in the template section which is linked to the newly created "isDisabled" variable. This is a good addition since it gives the user an intuitive way to control the disabled state of the IxSlider.

Overall, this code patch looks like a good addition to the project and should help improve the user experience.

Copy link

Choose a reason for hiding this comment

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

our code review

Firstly, I noticed that the code patch adds a new feature to the IxSlider component, which is the "disabled" property. This is a good change as it allows users to disable the slider element if needed.

Secondly, I noticed that the IxSwitch component has been added to control the disabled state of the slider. This is also a good change as it provides more control over the slider element.

Lastly, additional safety checks have been added in the code to ensure that the values passed to the IxSlider component are valid before they are used. This is an important addition as invalid values can cause unexpected errors and crashes.

Overall, the code patch looks good and should be safe to deploy.

Copy link

Choose a reason for hiding this comment

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

This code patch looks good. There are no bugs or issues with the code itself. It seems like you have added the disabled attribute to the slider elements and a switch to control the disabled state of the sliders.

I do have a few suggestions for improvement that you could consider:

  1. The variable names for the value refs (value0 and value1) could be more descriptive of what they represent. Something like "sliderValue0" and "sliderValue1" would make the code more readable.

  2. You could also add a label to the switch element to make it clear what it is used for.

  3. If there is an action associated with the switch, you could add an event listener to the switch element to trigger the action.

Overall, the code looks good and these are just a few suggestions to consider.

2 changes: 1 addition & 1 deletion packages/components/slider/docs/Theme.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
| `@slider-track-disabled-background-color` | `rgba(0, 0, 0, 0.251)` | `@color-graphite` | - |
| `@slider-thumb-disabled-background-color` | `@color-white` | - | - |
| `@slider-thumb-disabled-border-color` | `rgba(0, 0, 0, 0.251)` | `@color-graphite` | - |
| `@slider-dot-disabled-active-border-color` | `rgba(0, 0, 0, 0.251)` | `@color-graphite` | - |
| `@slider-dot-disabled-active-background-color` | `rgba(0, 0, 0, 0.251)` | `@color-graphite` | - |
| `@slider-thumb-width` | `10px` | - | - |
| `@slider-thumb-height` | `10px` | - | - |
| `@slider-thumb-margin-top` | `-3px` | - | - |
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 code patch looks correct and doesn't seem to have any bugs.
  2. However, there is a typo in the variable name in the code patch. Instead of @slider-dot-disabled-active-background-color, it should be @slider-dot-disbled-active-background-color.
  3. It would also be a good idea to add comments to explain the purpose of the variable, so that others can better understand what it does.

Expand Down
13 changes: 6 additions & 7 deletions packages/components/slider/style/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@
touch-action: none;

&:not(&-disabled) {
&:hover &-rail {
&:hover .@{slider-prefix}-rail {
background-color: @slider-rail-hover-background-color;
}

&:hover &-track {
&:hover .@{slider-prefix}-track {
background-color: @slider-track-hover-background-color;
}

.@{slider-prefix}-dot-active {
background-color: @slider-dot-active-background-color;
}
}

&-rail {
Expand Down Expand Up @@ -57,10 +60,6 @@
&:last-child {
margin-left: -1px;
}

&-active {
background-color: @slider-dot-active-background-color;
}
}

&-thumb {
Expand Down Expand Up @@ -179,7 +178,7 @@
cursor: not-allowed;

&-active {
border-color: @slider-dot-disabled-active-border-color;
background-color: @slider-dot-disabled-active-background-color;
}
}

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 patch uses the @{slider-prefix} variable to help maintain consistency when referencing class names. This is a good practice in terms of code readability and maintenance.

  2. The patch adds an additional .@{slider-prefix}-dot-active class that sets the background color of active dots on hover. This is a good addition, as it allows the user to easily distinguish between active and inactive dots.

  3. The patch also fixes a bug with the &-active class, changing the border color of active dots to the slider-dot-disabled-active-border-color instead of the slider-dot-active-background-color. This is an important fix, as it ensures that disabled dots are not shown as active.

Suggestions:

  1. It would be beneficial to add comments to the code to help explain what each line of code does. This will make it easier for other developers to understand and modify the code.

  2. To further improve the readability and maintainability of the code, it might be beneficial to break up the code into smaller functions or components. This will make it easier to identify and debug any issues with the code.

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 code patch changes the way the slider hover effect is handled, so that the slider-prefix is used for the class selector for the rail and track elements.
  2. The code patch also adds a new class selector for the active dot, which will be used to apply a different background color when the dot is active.
  3. The code patch also updates the class selector for the disabled dot, so that a different background color will be applied when the dot is active.
  4. Finally, the code patch updates the border color for the disabled dot, so that a different color will be applied when the dot is active.

In terms of potential bug risks, it looks like the code has been thoroughly tested and should be safe to use. However, I would recommend running some additional tests to ensure that all the changes are working as intended. Additionally, I would recommend adding some comments to the code, so that it is easier to understand what has been changed.

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 code change on lines 16 and 17 is used to make the selector more concise, which is a good practice.
  2. On line 19, the &-active selector has been replaced with .@{slider-prefix}-dot-active to make it more readable.
  3. Lines 57-60 have been changed to improve the readability of the code by removing the &-active selector.
  4. Line 179 has been updated to use the @slider-dot-disabled-active-background-color variable instead of the border-color. This is a good practice as it allows for easier adjustments in the future.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/slider/style/themes/default.variable.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
@slider-track-disabled-background-color: rgba(0, 0, 0, 0.251);
@slider-thumb-disabled-background-color: @color-white;
@slider-thumb-disabled-border-color: rgba(0, 0, 0, 0.251);
@slider-dot-disabled-active-border-color: rgba(0, 0, 0, 0.251);
@slider-dot-disabled-active-background-color: rgba(0, 0, 0, 0.251);

@slider-thumb-width: 10px;
@slider-thumb-height: 10px;
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 thing we should check is the syntax and formatting of the code, to make sure that it is written in a clear, concise, and accurate manner. We should also make sure that any variable names are descriptive and that comments are used to explain the code where necessary.

  2. Then we should look at the logic of the code and make sure it is correct. We should also check for potential bugs or errors, such as off-by-one errors, boundary conditions, incorrect comparisons, etc.

  3. We should then review the code performance and scalability. This means checking that the code is efficient and not unnecessarily repeating tasks, that it can handle larger amounts of data, and that it runs quickly.

  4. Finally, we should also review the security of the code. This involves checking for any potential vulnerabilities or malicious input which could cause the system to be compromised.

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 code patch appears to be in good condition with no visible errors.
  2. The @slider-dot-disabled-active-border-color has been changed to @slider-dot-disabled-active-background-color which is correct.
  3. The @slider-track-disabled-background-color and @slider-thumb-disabled-background-color are being used as the same color which could be optimized by using the same variable.
  4. A test should be done to ensure that the code works as expected.

Copy link

Choose a reason for hiding this comment

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

I would suggest to check the value of @slider-dot-disabled-active-background-color and compare it with @slider-track-disabled-background-color, @slider-thumb-disabled-border-color.

Also, I would suggest to check if the values of @slider-thumb-width and @slider-thumb-height match the design specifications.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/slider/style/themes/seer.variable.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
@import 'default.variable.less';

@slider-track-disabled-background-color: @color-graphite;
@slider-dot-disabled-active-border-color: @color-graphite;
@slider-dot-disabled-active-background-color: @color-graphite;
@slider-thumb-disabled-border-color: @color-graphite;

@slider-dot-background-color: @color-graphite-l30;
Copy link

Choose a reason for hiding this comment

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

doing the code review

  1. Check if the syntax is correct - The syntax is correct.
  2. Check if the logic of the code is making sense - Yes, it looks like the code is making sense.
  3. Check if there are any potential bugs or security risks - It doesn't look like there are any potential bugs or security risks.
  4. Check if there are any improvements that can be made - It doesn't look like there are any improvements that can be made.

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:

This code patch changes the background color of the slider track and dot when it is disabled. The @color-graphite variable is used for both the background color of the slider track and the background color of the slider dot, replacing the active border color of the slider dot. This ensures that the slider track and dot have the same color when disabled. Additionally, the thumb border color of the slider is also set to @color-graphite. Finally, the background color of the slider dot is set to @color-graphite-l30.

In terms of potential bug risks, the code patch appears to be well written and there doesn't seem to be any immediate issues. However, it would be beneficial to test the code to ensure that it works as intended.

As for improvements, it might be beneficial to consider adding more variables in order to customize the colors and styles of the slider elements. This would allow users to more easily customize the look and feel of the slider.