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:slider): slider marker color error when disabled #1457

Merged
merged 1 commit into from
Feb 23, 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?

激活状态的 mark,在禁用状态下仍然是蓝色
image

What is the new behavior?

修改样式,使mark在禁用状态下展示灰色

Other information

视觉检视问题

@@ -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.

@idux-bot
Copy link

idux-bot bot commented Feb 20, 2023

This preview will be available after the AzureCI is passed.

| `@slider-thumb-width` | `10px` | - | - |
| `@slider-thumb-height` | `10px` | - | - |
| `@slider-thumb-margin-top` | `-3px` | - | - |
| `@slider-thumb-vertical-margin-left` | `-4px` | - | - |
| `@slider-thumb-vertical-margin-top` | `-6px` | - | - |
| `@slider-height` | `2px` | - | - |
| `@slider-width` | `2px` | - | - |
| `@slider-dot-disabled-active-border-color` | - | `@color-graphite` | - |
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

First, there is a missing semi-colon at the end of the second line. Additionally, it would be good to check that all the variables used in this patch are properly declared and that they are being used as intended.

Next, there is a typo in the fourth line, where ‘border-color’ should be changed to ‘background-color’.

It would also be beneficial to add an extra line for clarity between each key-value pair, as this will make the code easier to read and understand.

Finally, it is important to check that all the changes are necessary and that none of the existing code has been removed or changed unnecessarily. It would be sensible to add a comment above each line to explain the purpose of the change. This will help to ensure that any future maintenance of this code is straightforward.

@@ -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.

@@ -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.

@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #1457 (755270c) into main (8ca7a08) will not change coverage.
The diff coverage is n/a.

❗ Current head 755270c differs from pull request most recent head 4cc2369. Consider uploading reports for the commit 4cc2369 to get more accurate results

@@           Coverage Diff           @@
##             main    #1457   +/-   ##
=======================================
  Coverage   92.89%   92.89%           
=======================================
  Files         327      327           
  Lines       30399    30399           
  Branches     3507     3507           
=======================================
  Hits        28238    28238           
  Misses       2161     2161           

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 Feb 20, 2023
@@ -16,11 +16,12 @@
| `@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)` | - | - |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `@slider-dot-disabled-active-background-color` | `rgba(0, 0, 0, 0.251)` | - | - |
| `@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` | - | - |
| `@slider-thumb-vertical-margin-left` | `-4px` | - | - |
| `@slider-thumb-vertical-margin-top` | `-6px` | - | - |
| `@slider-height` | `2px` | - | - |
| `@slider-width` | `2px` | - | - |
| `@slider-dot-disabled-active-border-color` | - | `@color-graphite` | - |
Copy link
Member

Choose a reason for hiding this comment

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

Seer 的变量名忘改了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

提交了

@sallerli1 sallerli1 force-pushed the fix-slider-marker-color-saller branch from 49b53f0 to 0ea2399 Compare February 20, 2023 09:07
@@ -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.

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.

| `@slider-thumb-width` | `10px` | - | - |
| `@slider-thumb-height` | `10px` | - | - |
| `@slider-thumb-margin-top` | `-3px` | - | - |
| `@slider-thumb-vertical-margin-left` | `-4px` | - | - |
| `@slider-thumb-vertical-margin-top` | `-6px` | - | - |
| `@slider-height` | `2px` | - | - |
| `@slider-width` | `2px` | - | - |
| `@slider-dot-disabled-active-border-color` | - | `@color-graphite` | - |
Copy link

Choose a reason for hiding this comment

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

with a code review.

The patch appears to add a new variable and update an existing variable within the codebase. The new variable is @slider-dot-disabled-active-background-color, which is set to rgba(0, 0, 0, 0.251) and has no default value. The existing variable @slider-dot-disabled-active-border-color is updated to use a default value of @color-graphite.

After a quick glance at the code, there doesn't appear to be any bugs associated with this patch. However, it is always important to thoroughly test any changes to ensure that all expected behaviors are working as expected. Additionally, it may also be beneficial to review any existing documentation related to these variables to ensure that any updates are accurately reflected in the documentation.

@@ -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.

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.

@@ -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.

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.

@@ -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.

@sallerli1 sallerli1 force-pushed the fix-slider-marker-color-saller branch from 0ea2399 to 4cc2369 Compare February 20, 2023 09:08
@@ -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.

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.

@@ -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.

@@ -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.

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.

@@ -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.

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.

@@ -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.

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.

@danranVm danranVm merged commit d215935 into IDuxFE:main Feb 23, 2023
@sallerli1 sallerli1 deleted the fix-slider-marker-color-saller branch July 4, 2024 09:28
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