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(module:slider): fix slider style + reverse marks if reversed slider #6006

Merged
merged 2 commits into from Dec 31, 2020

Conversation

KeelanS
Copy link
Contributor

@KeelanS KeelanS commented Nov 3, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently the marks and steps of sliders don't get updated when the slider get reversed. When you reverse a slider with marks, the '0' mark will be under value 100 and the '100' mark will be under value 0. The same thing happens for steps, when you have slider value 50 the '0' step doesn't show as 'active', while the '100' step does.

Issue Number: N/A

What is the new behavior?

Now the marks get built while taking the reversed state of the slider into account. The marks also stay under the right value.

The main issue I wanted to solve here is the styling of the steps. But while testing I saw that the marks didn't do what I was expecting either, so I added the functionality that they stay under the right value. If you think this isn't what the marks should do, please tell me so that I can remove it from this PR.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@zorro-bot
Copy link

zorro-bot bot commented Nov 3, 2020

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #6006 (da9ee3b) into master (50f0744) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6006      +/-   ##
==========================================
- Coverage   89.95%   89.90%   -0.06%     
==========================================
  Files         465      465              
  Lines       14044    14063      +19     
  Branches     2307     2313       +6     
==========================================
+ Hits        12633    12643      +10     
- Misses        863      871       +8     
- Partials      548      549       +1     
Impacted Files Coverage Δ
components/slider/slider.component.ts 91.39% <ø> (ø)
components/slider/marks.component.ts 89.47% <100.00%> (+0.28%) ⬆️
components/slider/step.component.ts 90.00% <100.00%> (+2.00%) ⬆️
components/table/src/table-style.service.ts 94.82% <0.00%> (-1.73%) ⬇️
components/tabs/tab-nav-bar.component.ts 80.91% <0.00%> (-1.42%) ⬇️
components/tabs/tabset.component.ts 86.20% <0.00%> (ø)
components/typography/typography.component.ts 97.82% <0.00%> (+0.03%) ⬆️
components/select/select.component.ts 87.70% <0.00%> (+0.16%) ⬆️
components/select/select-top-control.component.ts 89.28% <0.00%> (+0.39%) ⬆️
components/code-editor/code-editor.component.ts 14.78% <0.00%> (+1.05%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50f0744...da9ee3b. Read the comment docs.

Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

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

LGTM. Better to add some tests. 😄

@wzhudev wzhudev added the PR: need-test Test is necessary for code changes. label Nov 9, 2020
@KeelanS KeelanS requested a review from wzhudev November 16, 2020 18:35
@KeelanS
Copy link
Contributor Author

KeelanS commented Nov 30, 2020

@wendellhu95 I added some tests, can you review this again? Thanks!

Copy link
Contributor

@DepickereSven DepickereSven left a comment

Choose a reason for hiding this comment

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

LGTM and there are even tests now.

@DepickereSven
Copy link
Contributor

@wendellhu95 or @hsuanxyz Is it possible to review this PR again?

Copy link
Member

@hsuanxyz hsuanxyz left a comment

Choose a reason for hiding this comment

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

LGTM, @wendellhu95

Copy link
Contributor

@yangjunhan yangjunhan left a comment

Choose a reason for hiding this comment

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

LGTM~

@yangjunhan yangjunhan merged commit fa06415 into NG-ZORRO:master Dec 31, 2020
@pr-triage pr-triage bot added the PR: merged label Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants