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

Pass id prop to DatePicker input field #8598

Merged
merged 1 commit into from Jan 24, 2018
Merged

Conversation

@mgrdevport
Copy link
Contributor

@mgrdevport mgrdevport commented Dec 13, 2017

This PR passes the DatePicker prop id to the rendered input field as requested in #8305.

@codecov
Copy link

@codecov codecov bot commented Dec 13, 2017

Codecov Report

Merging #8598 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8598   +/-   ##
=======================================
  Coverage   85.25%   85.25%           
=======================================
  Files         195      195           
  Lines        4674     4674           
  Branches     1378     1378           
=======================================
  Hits         3985     3985           
  Misses        689      689
Impacted Files Coverage Δ
components/date-picker/WeekPicker.tsx 78.72% <ø> (ø) ⬆️
components/date-picker/interface.tsx 0% <ø> (ø) ⬆️
components/date-picker/RangePicker.tsx 94.69% <ø> (ø) ⬆️
components/date-picker/createPicker.tsx 92.59% <ø> (ø) ⬆️

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 cdb160a...9e42be9. Read the comment docs.

@benjycui
Copy link
Contributor

@benjycui benjycui commented Dec 14, 2017

Could you update RangePicker at the same time?

@yesmeck
Copy link
Member

@yesmeck yesmeck commented Dec 14, 2017

@benjycui
Copy link
Contributor

@benjycui benjycui commented Dec 15, 2017

@yesmeck actually.. I just think this could work with label[for]

P.S. we just need to make DatePicker work with onFocus

@yesmeck
Copy link
Member

@yesmeck yesmeck commented Dec 15, 2017

@benjycui 👍

@mgrdevport mgrdevport force-pushed the mgrdevport:datepicker branch 2 times, most recently Dec 19, 2017
@mgrdevport
Copy link
Contributor Author

@mgrdevport mgrdevport commented Dec 19, 2017

To have the id on the initial input field would help us to write better dom selectors while testing:
findById->focus->findByClass(.ant-calendar-input)->setValue
Better than:
findByClass(.ant-calendar-picker-input)->focus[n]->findByClass(.ant-calendar-input)->setValue

@benjycui
Copy link
Contributor

@benjycui benjycui commented Dec 19, 2017

Conflict....

@mgrdevport mgrdevport force-pushed the mgrdevport:datepicker branch Dec 19, 2017
@mgrdevport
Copy link
Contributor Author

@mgrdevport mgrdevport commented Dec 20, 2017

conflicts are resolved, tests are running fine locally. I don't know why the snapshots of collapse are failing on the travis ci build.

@yesmeck
Copy link
Member

@yesmeck yesmeck commented Dec 20, 2017

@mgrdevport Rebase master

@mgrdevport mgrdevport force-pushed the mgrdevport:datepicker branch 2 times, most recently Dec 20, 2017
@mgrdevport
Copy link
Contributor Author

@mgrdevport mgrdevport commented Dec 20, 2017

@yesmeck Thanks

components/date-picker/RangePicker.tsx Outdated
@@ -267,6 +267,7 @@ export default class RangePicker extends React.Component<any, any> {
disabled={props.disabled}
readOnly
value={(start && start.format(props.format)) || ''}
id={props.id && `${props.id}-start`}

This comment has been minimized.

@benjycui

benjycui Dec 21, 2017
Contributor

We should add id in the wrapper, instead inputs..

This comment has been minimized.

@mgrdevport

mgrdevport Dec 29, 2017
Author Contributor

That would mean we would have to search for the first and second input inside the wrapper:
findById(myid)->find(input)[nth]

What do you think of adding an additional id to the wrapper, without the start and end postfix?
findById(myid)->find(input#myid-start)
findById(myid)->find(input#myid-end)

This comment has been minimized.

@yutingzhao1991

yutingzhao1991 Dec 29, 2017
Contributor

@yesmeck What your opinion. I will release a new version soon.

This comment has been minimized.

@yesmeck

yesmeck Dec 29, 2017
Member

Agree with @benjycui

@yesmeck
Copy link
Member

@yesmeck yesmeck commented Jan 13, 2018

@mgrdevport mgrdevport force-pushed the mgrdevport:datepicker branch Jan 16, 2018
@mgrdevport
Copy link
Contributor Author

@mgrdevport mgrdevport commented Jan 16, 2018

I've moved the id declaration to the wrapper.

@ddcat1115
Copy link
Contributor

@ddcat1115 ddcat1115 commented Jan 20, 2018

Conflict... Please rebase master again @mgrdevport

@mgrdevport mgrdevport force-pushed the mgrdevport:datepicker branch Jan 21, 2018
@mgrdevport
Copy link
Contributor Author

@mgrdevport mgrdevport commented Jan 21, 2018

@ddcat1115 done.

@mgrdevport mgrdevport force-pushed the mgrdevport:datepicker branch to 9e42be9 Jan 22, 2018
@yesmeck yesmeck merged commit 38bf0be into ant-design:master Jan 24, 2018
4 checks passed
4 checks passed
@codecov
codecov/patch Coverage not affected when comparing cdb160a...9e42be9
Details
@codecov
codecov/project 85.25% remains the same compared to cdb160a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new issues
Details
yesmeck added a commit that referenced this pull request Jan 24, 2018
This reverts commit 38bf0be.
yesmeck added a commit that referenced this pull request Jan 24, 2018
@mgrdevport mgrdevport deleted the mgrdevport:datepicker branch Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants