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: calendar picker view scroll #6521

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Yueyanc
Copy link

@Yueyanc Yueyanc commented Jan 11, 2024

测试有两个未通过,不太清楚为什么。
image
image

@Yueyanc
Copy link
Author

Yueyanc commented Jan 11, 2024

#6425

@Yueyanc
Copy link
Author

Yueyanc commented Jan 12, 2024

@zombieJ 这块这样实现可行吗,可行的话我添加文档和修复用例报错

<div
className={classNames(
`${classPrefix}-cell-date`,
`${classPrefix}-cell-${d.format('YYYY-MM-DD')}`
Copy link
Member

Choose a reason for hiding this comment

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

不要加到 className 里,可以加个 data-date

Copy link
Contributor

PR preview has been successfully built and deployed to https://antd-mobile-preview-pr-6521.surge.sh

@Yueyanc Yueyanc requested a review from zombieJ January 13, 2024 13:46
@Yueyanc
Copy link
Author

Yueyanc commented Jan 16, 2024

@zombieJ 帮忙review一下呢

@bravepg
Copy link
Contributor

bravepg commented Jan 16, 2024

感觉这个 api 有点奇怪。是不是打开弹窗之后最好可以直接定位到选择的日期,而不是再调用个 api 来滚动到相应的位置?

@Yueyanc
Copy link
Author

Yueyanc commented Jan 16, 2024

感觉这个 api 有点奇怪。是不是打开弹窗之后最好可以直接定位到选择的日期,而不是再调用个 api 来滚动到相应的位置?

弹窗打开的时候内部调用了这个api 不需要再手动调用了

@bravepg
Copy link
Contributor

bravepg commented Jan 16, 2024

感觉这个 api 有点奇怪。是不是打开弹窗之后最好可以直接定位到选择的日期,而不是再调用个 api 来滚动到相应的位置?

弹窗打开的时候内部调用了这个api 不需要再手动调用了

image
那这里就可以不用暴露了吧?

@Yueyanc
Copy link
Author

Yueyanc commented Jan 16, 2024

感觉这个 api 有点奇怪。是不是打开弹窗之后最好可以直接定位到选择的日期,而不是再调用个 api 来滚动到相应的位置?

弹窗打开的时候内部调用了这个api 不需要再手动调用了

image 那这里就可以不用暴露了吧?

这里是因为考虑到可能有用户想要定位到其他日期才添加上去的。需要去掉吗

@Yueyanc
Copy link
Author

Yueyanc commented Jan 16, 2024

感觉这个 api 有点奇怪。是不是打开弹窗之后最好可以直接定位到选择的日期,而不是再调用个 api 来滚动到相应的位置?

弹窗打开的时候内部调用了这个api 不需要再手动调用了

image 那这里就可以不用暴露了吧?

scrollTo这个api是通过ref传出来的,如果文档里面不写,但是用户在ref里面能看见这个api,会不会有疑惑

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b431a18) 92.27% compared to head (45cd1d1) 92.29%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6521      +/-   ##
==========================================
+ Coverage   92.27%   92.29%   +0.02%     
==========================================
  Files         316      316              
  Lines        6897     6906       +9     
  Branches     1728     1733       +5     
==========================================
+ Hits         6364     6374      +10     
+ Misses        497      496       -1     
  Partials       36       36              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zombieJ
Copy link
Member

zombieJ commented Jan 22, 2024

@bravepg,印象里渲染面板数量有限。这个 scrollTo 感觉容易超出范围经常不生效。确认一下。

@Jarryxin
Copy link
Contributor

Jarryxin commented Jan 22, 2024

这个实现有些缺陷,理由如下:

  1. scrollTo后只是将要展示的日期显示到屏幕中间(调用 scrollIntoView({ block: 'center' })),展示的截断区域比较奇怪,如下,最好是将当前月完整显示
    image
  2. data-date加cell日期的方式只改了默认cell显示的属性,renderDate自定义日期渲染的情况未处理
  3. 当设置了min后,默认未从defaultValue/value设置的日期开始渲染,当 visible 为 true 时,calendarRef.current不能立刻取到,需要加setImmediate获取 calendarRef

@zombieJ @Yueyanc

@@ -73,7 +73,12 @@ export const CalendarPicker = forwardRef<
getContainer,
...calendarViewProps
} = props

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

useEffect(() => {
    setImmediate(() => {
      const dateRange = calendarRef.current?.getDateRange() ?? null
      if (dateRange && dateRange[0]) {
        calendarRef.current?.scrollTo(dateRange[0])
      }
    })
  }, [visible])

加setImmediate, 当 visible 为 true 时,calendarRef.current不能立刻取到

@Yueyanc
Copy link
Author

Yueyanc commented Jan 22, 2024

这个实现有些缺陷,理由如下:

  1. scrollTo后只是将要展示的日期显示到屏幕中间(调用 scrollIntoView({ block: 'center' })),展示的截断区域比较奇怪,如下,最好是将当前月完整显示

image

  1. data-date加cell日期的方式只改了默认cell显示的属性,renderDate自定义日期渲染的情况未处理

  2. 当设置了min后,默认未从defaultValue/value设置的日期开始渲染,当 visible 为 true 时,calendarRef.current不能立刻取到,需要加setImmediate获取 calendarRef

@zombieJ @Yueyanc

🤡我当时没想这么多
@bravepg 老哥怎么看

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.

4 participants