-
Notifications
You must be signed in to change notification settings - Fork 140
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:slider): slider marker color error when disabled #1457
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"> | ||
|
@@ -23,4 +25,5 @@ const marks = { | |
|
||
const value0 = ref(30) | ||
const value1 = ref(50) | ||
const isDisabled = ref(false) | ||
</script> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Overall, the code looks good and these are just a few suggestions to consider. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | - | - | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -57,10 +60,6 @@ | |
&:last-child { | ||
margin-left: -1px; | ||
} | ||
|
||
&-active { | ||
background-color: @slider-dot-active-background-color; | ||
} | ||
} | ||
|
||
&-thumb { | ||
|
@@ -179,7 +178,7 @@ | |
cursor: not-allowed; | ||
|
||
&-active { | ||
border-color: @slider-dot-disabled-active-border-color; | ||
background-color: @slider-dot-disabled-active-background-color; | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Review:
Suggestions:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with a brief code review:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with a brief code review
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the code review
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Review:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doing the code review
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
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.
the code review:
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.
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.
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.