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(Anchor): add targetOffset prop #17827

Merged
merged 30 commits into from Aug 17, 2019

Conversation

@shaodahong
Copy link
Collaborator

commented Jul 23, 2019

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

#17663

💡 Background and solution

After fixed nav, scroll to anchor target, it's no visible.
固定导航后,滚动到锚点的目标被遮挡

📝 Changelog

Language Changelog
🇺🇸 English Anchor add targetOffset to support customize scroll position offset.
🇨🇳 Chinese Anchor 添加 targetOffset 以支持自定义滚动偏移量。

☑️ Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@pr-triage pr-triage bot added the PR: unreviewed label Jul 23, 2019

@netlify

This comment has been minimized.

Copy link

commented Jul 23, 2019

Deploy preview for ant-design ready!

Built with commit 4968052

https://deploy-preview-17827--ant-design.netlify.com

@buildsize

This comment has been minimized.

Copy link

commented Jul 23, 2019

File name Previous Size New Size Change
package-lock.json 817.42 KB 820.79 KB 3.37 KB (0%)
@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

@zombieJ Please check this PR

@codecov

This comment has been minimized.

Copy link

commented Jul 23, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (feature@161def0). Click here to learn what that means.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             feature   #17827   +/-   ##
==========================================
  Coverage           ?   96.04%           
==========================================
  Files              ?      268           
  Lines              ?     7457           
  Branches           ?     2073           
==========================================
  Hits               ?     7162           
  Misses             ?      293           
  Partials           ?        2
Impacted Files Coverage Δ
components/anchor/Anchor.tsx 90% <75%> (ø)

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 161def0...13b6f6f. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Jul 23, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (feature@4e26107). Click here to learn what that means.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             feature   #17827   +/-   ##
==========================================
  Coverage           ?   96.26%           
==========================================
  Files              ?      280           
  Lines              ?     7523           
  Branches           ?     2050           
==========================================
  Hits               ?     7242           
  Misses             ?      279           
  Partials           ?        2
Impacted Files Coverage Δ
components/_util/scrollTo.ts 100% <100%> (ø)
components/_util/easings.ts 100% <100%> (ø)
components/back-top/index.tsx 67.34% <75%> (ø)
components/anchor/Anchor.tsx 94.4% <95.23%> (ø)

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 4e26107...4968052. Read the comment docs.

components/anchor/Anchor.tsx Outdated Show resolved Hide resolved
@zombieJ

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Seems ok. Could you help to add a test case about this?

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

In my see, because unexport scrollTo function so too hard to test targetOffset prop, an easy case like(useless):

expect(wrapper.prop('targetOffset')).toBe(some value);

I think we should be split scrollTo function to utils module

@zombieJ

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Here is some useful util of (domHook)[https://github.com/ant-design/ant-design/blob/master/components/tests/util/domHook.js] we current use to mock the HTML values. You can have a try~

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 24, 2019

thank you, domHook is useful, I write a test case, and modify easeInOutCubic function return value, it's last call return value more than the target

@zombieJ

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

So efficiency! Could you add a demo about it and add link in targetOffset description makes user easy to understand?

components/anchor/Anchor.tsx Outdated Show resolved Hide resolved
@todo

This comment has been minimized.

Copy link

commented Jul 24, 2019

support x

// TODO: support x
export default function scrollTo(x: number, y: number, options: ScrollToOptions = {}) {
const {
getContainer = () => window,
callback,
duration = 450,


This comment was generated by todo based on a TODO comment in 2d9deae in #17827. cc @shaodahong.
components/_util/easings.ts Outdated Show resolved Hide resolved
components/_util/scrollTo.ts Outdated Show resolved Hide resolved
components/_util/easings.ts Outdated Show resolved Hide resolved
@zombieJ

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

getCurrentAnchor 是否需要调整一下?有了 offsetTarget 后感觉应该和这个值相关而非 scrollTop 相关。 @afc163 @shaodahong

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 2, 2019

getCurrentAnchor 和 targetOffset 没什么关联吧,是用来设置高亮锚点链接用的,倒是和 getOffsetTop 有关系,当时倒是想把 getOffsetTop 抽出去,不过看了下没有复用的

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

有冲突,我 rebase 了下

@afc163 @zombieJ #17823 这个新增的 prop 是不是版本号不对,应该也是 3.22.0 吧

@afc163

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

我发布前改掉。

@afc163
afc163 approved these changes Aug 17, 2019

@afc163 afc163 merged commit 92b36c0 into ant-design:feature Aug 17, 2019

25 of 27 checks passed

Header rules - ant-design No header rules processed
Details
Pages changed - ant-design 214 new files uploaded
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
Mixed content - ant-design No mixed content detected
Details
Redirect rules - ant-design 18 redirect rules processed
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
ant-design.ant-design #ant design succeeded
Details
buildsize No change
Details
ci/circleci: compile Your tests passed on CircleCI!
Details
ci/circleci: dist Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_dist Your tests passed on CircleCI!
Details
ci/circleci: test_dist_15 Your tests passed on CircleCI!
Details
ci/circleci: test_dom Your tests passed on CircleCI!
Details
ci/circleci: test_dom_15 Your tests passed on CircleCI!
Details
ci/circleci: test_es Your tests passed on CircleCI!
Details
ci/circleci: test_es_15 Your tests passed on CircleCI!
Details
ci/circleci: test_lib Your tests passed on CircleCI!
Details
ci/circleci: test_lib_15 Your tests passed on CircleCI!
Details
ci/circleci: test_node Your tests passed on CircleCI!
Details
ci/circleci: test_node_15 Your tests passed on CircleCI!
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
netlify/ant-design/deploy-preview Deploy preview ready!
Details
security/snyk - package.json (paranoidjk) No manifest changes detected

@pr-triage pr-triage bot added PR: merged and removed PR: unreviewed labels Aug 17, 2019

const frameFunc = () => {
const timestamp = Date.now();
const time = timestamp - startTime;
this.setScrollTop(easeInOutCubic(time, scrollTop, 0, 450));

This comment has been minimized.

Copy link
@afc163

afc163 Aug 22, 2019

Member

setScrollTop 和 getCurrentScrollTop 已经没有用了,但是没删,导致测试覆盖率下降。

在这里 #18406 修复。

This comment has been minimized.

Copy link
@shaodahong

shaodahong Aug 22, 2019

Author Collaborator

好的

@afc163

This comment has been minimized.

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 28, 2019

我本地换了几个 node 版本单独跑了下是 OK 的

yarn test --runInBand components/anchor/__tests__/Anchor.test.js
@afc163

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

本地都是好的,发现换 circleci 的 image 就不行了,只有 node:8 能过,其他的基本有一个必挂。

9d2f62b

@zombieJ

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

是不是又和 lodash 的 debounce 有关?

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

@zombieJ 不清楚,最近忙,等周末有空看看,可能是我写的 dateMock 有问题吧,主要是本地没办法复现不好测试

@shaodahong

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

@zombieJ 自己测试了下,发现可能出现的情况

  1. 把 dateMock 提前消费掉了
  2. dateMock 没有 mock 成功

之后我改进下 dateMock 避免被提前消费。如果后面还有问题,就说明是第 2 个原因了

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.