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(InputItem): add try catch to getSelection (#3236) #3237

Merged
merged 2 commits into from Jun 12, 2019

Conversation

zack24q
Copy link
Contributor

@zack24q zack24q commented May 28, 2019

First of all, thank you for your contribution! :-)

Please makes sure that these checkboxes are checked before submitting your PR, thank you!

  • Make sure that you follow antd's code convention.
  • Run npm run lint and fix those errors before submitting in order to keep consistent code style.
  • Rebase before creating a PR to keep commit history clear.
  • Add some descriptions and refer relative issues for you PR.

Extra checklist:

if isBugFix :

  • Make sure that you add at least one unit test for the bug which you had fixed.

elif isNewFeature :

  • Update API docs for the component.
  • Update/Add demo to demonstrate new feature.
  • Update TypeScript definition for the component.
  • Add unit tests for the feature.

This change is Reviewable

@zack24q zack24q requested review from ziluo and doxiaodong May 28, 2019 13:50
@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

Merging #3237 into master will increase coverage by 0.02%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3237      +/-   ##
=========================================
+ Coverage   60.07%   60.1%   +0.02%     
=========================================
  Files          92      92              
  Lines        2282    2286       +4     
  Branches      656     656              
=========================================
+ Hits         1371    1374       +3     
- Misses        902     903       +1     
  Partials        9       9
Flag Coverage Δ
#web 60.1% <83.33%> (+0.02%) ⬆️
Impacted Files Coverage Δ
components/input-item/index.tsx 64.19% <83.33%> (+0.27%) ⬆️
components/locale-provider/locale-provider.tsx 100% <0%> (ø) ⬆️

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 203eaa8...698f129. Read the comment docs.


let prePos = 0;
try {
prePos = el.selectionEnd || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

备注下为啥会异常?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

感觉这样写会比较直观。

try {
  
      prePos = el.selectionEnd;
} catch (e) {
   prePos = 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这样子改TS会报错:不能将类型“number | null”分配给类型“number”

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
Contributor

Choose a reason for hiding this comment

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

catch到错误只是console.warn么,没有兜底措施吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

兜底就是不处理光标位置了

@doxiaodong
Copy link
Contributor

这个问题改了好几次了的样子。。

@doxiaodong doxiaodong merged commit 4f12100 into master Jun 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-input-item-error branch June 12, 2019 17:22
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.

None yet

4 participants