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(carousel): add arrow prop #451

Merged
merged 52 commits into from
Jul 9, 2021
Merged

Conversation

doom-9-zz
Copy link
Contributor

@doom-9-zz doom-9-zz commented Jul 8, 2021

close #410

doom-9 and others added 30 commits June 17, 2021 17:42
@vercel
Copy link

vercel bot commented Jul 8, 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/99XFPKRscMJne582RkheFhaA4UgF
✅ Preview: https://naive-ui-git-fork-doom-9-featcarousel-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #451 (16353b2) into main (f64d37a) will increase coverage by 0.07%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   36.72%   36.79%   +0.07%     
==========================================
  Files         507      507              
  Lines       12164    12169       +5     
  Branches     3342     3345       +3     
==========================================
+ Hits         4467     4478      +11     
+ Misses       6835     6828       -7     
- Partials      862      863       +1     
Impacted Files Coverage Δ
src/carousel/src/Carousel.tsx 30.00% <80.00%> (+6.55%) ⬆️

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 f64d37a...16353b2. Read the comment docs.

@07akioni
Copy link
Collaborator

07akioni commented Jul 8, 2021

@doom-9 close #410

需要这么写

Copy link
Collaborator

@07akioni 07akioni left a comment

Choose a reason for hiding this comment

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

来个测试,把新增的覆盖一下

@@ -12,6 +12,7 @@
### Feats

- `n-tree` exports `TreeDragInfo` & `TreeDropInfo` type.
- `n-carousel` add `arrow` prop.
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
- `n-carousel` add `arrow` prop.
- `n-carousel` add `show-arrow` prop.

@@ -12,6 +12,7 @@
### Feats

- `n-tree` 导出 `TreeDragInfo` & `TreeDropInfo` 类型
- `n-carousel` 新增 `arrow` 属性
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
- `n-carousel` 新增 `arrow` 属性
- `n-carousel` 新增 `show-arrow` 属性

@@ -17,6 +18,7 @@ dot-placement

| Name | Type | Default | Description |
| --- | --- | --- | --- |
| arrow | `boolean` | `false` | Arrow button. |
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
| arrow | `boolean` | `false` | Arrow button. |
| show-arrow | `boolean` | `false` | Whether to show arrow button. |

@@ -17,6 +18,7 @@ dot-placement

| 名称 | 类型 | 默认值 | 说明 |
| --- | --- | --- | --- |
| arrow | `boolean` | `false` | 显示箭头按钮 |
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
| arrow | `boolean` | `false` | 显示箭头按钮 |
| show-arrow | `boolean` | `false` | 是否显示箭头按钮 |

@@ -350,6 +353,62 @@ export default defineComponent({
)
})}
</div>
{arrow && !vertical && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

arrow && [
btn1 class = vertical ? right : top,
btn2 class = vertical ? right : top,
]
大概就这个意思,减少重复代码

Comment on lines 64 to 65
height: 30%;
width: 8%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

我们还是先弄成固定大小吧


for (const item of dotToArrow) {
for (const dotItem of item.dot) {
await wrapper.setProps({ showArrow: true, dotPlacement: dotItem })
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.

done.

.trigger('click')
.then(async () => {
expect(slidesDOMArray[2].attributes('aria-hidden')).toBe('false')
await sleep(1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

话说这个应该有办法绕过去

@07akioni 07akioni merged commit b3a3497 into tusen-ai:main Jul 9, 2021
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.

Carousel component enhancements
4 participants