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

feat(slider): support vertical slider #1484

Merged
merged 14 commits into from
Nov 14, 2021

Conversation

nooooooom
Copy link
Contributor

close #1468.

@vercel
Copy link

vercel bot commented Oct 31, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/HhNPMUmAbqx98USh973uzmyQt3b1
✅ Preview: https://naive-ui-git-fork-nooooooom-feat-vertical-slider-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Oct 31, 2021

Codecov Report

Merging #1484 (af31772) into main (e59f9a8) will increase coverage by 0.39%.
The diff coverage is 31.44%.

❗ Current head af31772 differs from pull request most recent head 61ff429. Consider uploading reports for the commit 61ff429 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1484      +/-   ##
==========================================
+ Coverage   64.27%   64.66%   +0.39%     
==========================================
  Files         885      885              
  Lines       17122    16981     -141     
  Branches     4048     3995      -53     
==========================================
- Hits        11005    10981      -24     
+ Misses       5379     5263     -116     
+ Partials      738      737       -1     
Impacted Files Coverage Δ
src/slider/src/styles/index.cssr.ts 100.00% <ø> (ø)
src/slider/styles/_common.ts 100.00% <ø> (ø)
src/slider/src/Slider.tsx 37.24% <31.08%> (+7.35%) ⬆️
src/slider/src/utils.ts 50.00% <100.00%> (ø)

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 e59f9a8...61ff429. Read the comment docs.

@nooooooom
Copy link
Contributor Author

nooooooom commented Oct 31, 2021

Sorry I forgot to deal with mark, I will deal with it soon.

@@ -129,13 +127,22 @@ export default defineComponent({
uncontrolledValueRef
)

const wittyPlacementRef = computed(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const wittyPlacementRef = computed(() => {
const mergedPlacementRef = computed(() => {

Follow the covention of the library.

@@ -1,5 +1,6 @@
export default {
railHeight: '4px',
verticalRailWidth: '4px',
Copy link
Collaborator

Choose a reason for hiding this comment

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

railWidthVertical

表示状态的往后放,这是个对样式变量命名的最佳实践

margin-left, margin-right, ...etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解了

border-radius: var(--indicator-border-radius);
color: var(--indicator-text-color);
background-color: var(--indicator-color);
box-shadow: var(--indicator-box-shadow);
`, [
fadeInScaleUpTransition()
]),
c('.v-binder-follower-content', [
Copy link
Collaborator

Choose a reason for hiding this comment

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

这种样式的编写的时候不要依赖第三方库的类,不然那边一改就完犊子了,要内聚

Copy link
Collaborator

Choose a reason for hiding this comment

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

用在这个库里面的类当 content 的类

Copy link
Contributor Author

Choose a reason for hiding this comment

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

要不我根据 placement 自定义个类名

Copy link
Collaborator

Choose a reason for hiding this comment

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

placement 可以用 v-follower 那个,那个算是 vueuc 对外暴露的稳定接口

}
]}
style={this.cssVars as CSSProperties}
onKeydown={this.handleKeyDown}
// @ts-expect-error
Copy link
Collaborator

Choose a reason for hiding this comment

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

capture 这个 PR 半年多了,vue 还没合

Copy link
Collaborator

Choose a reason for hiding this comment

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

...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我看官网推荐我这么捕获,我换个写法吧,或者有更好的处理方式?主要是为了解决拖拽跟点击事件冲突了,拖拽结束之后还会触发一次点击预判,导致有时候拖拽不准

Copy link
Collaborator

Choose a reason for hiding this comment

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

没事,这样就可以,我也忘了 capture 我是咋处理的了

src/slider/src/Slider.tsx Show resolved Hide resolved
@@ -305,18 +339,28 @@ export default defineComponent({
}
}
}
function handleRailMouseDown (): void {
handleClickedRailRef.value = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

draggingRef 更符合语义吧?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的,我之前用的 draggingRef,但是看了下其它两个的命名就改过来了

@@ -28,5 +30,7 @@ format
| range | `boolean` | `false` | Whether the slider uses range value. |
| step | `number` | `1` | Step of the slider. |
| tooltip | `boolean` | `true` | Whether to show tooltip. |
| inverted | `boolean` | `false` | Whether to inverted the track. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个叫 reverse 比较好

@@ -1 +1 @@
export type OnUpdateValueImpl = (value: number | [number, number]) => void
export type OnUpdateValueImpl = (value: number | number[]) => void
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是为啥?还能有很多个 value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤣做着做着感觉可以处理多个value,就放开出来了,虽然没想到具体的应用场景

Copy link
Collaborator

Choose a reason for hiding this comment

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

这不太行,focus 状态太难处理了

Copy link
Collaborator

Choose a reason for hiding this comment

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

没事 我帮你改回去

Copy link
Collaborator

Choose a reason for hiding this comment

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

而且想不到有什么业务场景

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那就辛苦你了,今天终于有时间来做了

Copy link
Collaborator

Choose a reason for hiding this comment

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

我也是今天,赶紧收集一下之前的 PR

@07akioni 07akioni merged commit 82c8b33 into tusen-ai:main Nov 14, 2021
@nooooooom nooooooom deleted the feat-vertical-slider branch November 21, 2021 04:43
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.

slider enhancements - Support label and vertical scroll bar
2 participants