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(component): Added Date Time Picker component #1420

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@MariaAga
Copy link
Contributor

MariaAga commented Feb 21, 2019

What: Add the DateTimePicker component from PF to PF-react.

The PatternFly design is at:https://www.patternfly.org/pattern-library/forms-and-controls/date-and-time/#design

@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch from dfb287d to 149828d Feb 21, 2019
Copy link
Contributor

TheRealJon left a comment

There is one bug that I found:

If the currently selected day is today and you change the month and click the 'Today' button, the month does not switch back to the current month. I would expect that clicking 'Today' will always bring 'Today' back into view, whether it is already selected or not.

Most of my other comments are pretty minor suggestions that are obviously up for further discussion. Thanks for doing this.

@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented Feb 24, 2019

@TheRealJon Thanks for all the comment! The bug you described is still happening and will be fixed.
I've added a Time Picker, but this is still a WIP so the code is not completely 'clean'

@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch 2 times, most recently from 3a2b65f to c370ed5 Feb 25, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 26, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@22d777e). Click here to learn what that means.
The diff coverage is 91.75%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1420   +/-   ##
========================================
  Coverage          ?   81.2%           
========================================
  Files             ?     662           
  Lines             ?    8067           
  Branches          ?     505           
========================================
  Hits              ?    6551           
  Misses            ?    1276           
  Partials          ?     240
Flag Coverage Δ
#patternfly3 85.5% <91.75%> (?)
#patternfly4 76.28% <ø> (?)
#patternflymisc 95.68% <ø> (?)
Impacted Files Coverage Δ
...s/DateTimePicker/DateComponents/DecadeViewTable.js 100% <100%> (ø)
...components/DateTimePicker/DateComponents/Header.js 100% <100%> (ø)
...nents/DateTimePicker/DateComponents/TodayButton.js 100% <100%> (ø)
...mponents/DateTimePicker/DateComponents/YearView.js 100% <100%> (ø)
.../DateTimePicker/DateComponents/DecadeViewHeader.js 100% <100%> (ø)
...onents/DateTimePicker/DateComponents/DecadeView.js 100% <100%> (ø)
...omponents/DateTimePicker/DateComponents/helpers.js 100% <100%> (ø)
...rc/components/DateTimePicker/DateComponents/Day.js 100% <100%> (ø)
...nts/DateTimePicker/DateComponents/DateConstants.js 100% <100%> (ø)
...ponents/DateTimePicker/DateComponents/DateInput.js 72.72% <72.72%> (ø)
... and 3 more

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 22d777e...e732b69. Read the comment docs.

@MariaAga MariaAga changed the title [WIP]feat(component): Added Date Time Picker component feat(component): Added Date Time Picker component Feb 26, 2019
@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch 4 times, most recently from dafef6a to f4a5894 Feb 26, 2019
@laviro

This comment has been minimized.

Copy link
Collaborator

laviro commented Mar 3, 2019

@patternfly/patternfly-react-ux could you have a look? thanks

Copy link
Collaborator

laviro left a comment

Amazing work @MariaAga !!
Left some inline comments :)

state = {
value: new Date(this.props.value)
};
formatDate = () => {

This comment has been minimized.

Copy link
@laviro

laviro Mar 3, 2019

Collaborator

this function has been used in several places over the code,
could you move it into an helper file and extract it from there ?

This comment has been minimized.

Copy link
@MariaAga

MariaAga Mar 7, 2019

Author Contributor

Its different in every place (one for date, one for time, one for date-time) , and exporting it and adding its different variations will make it too complicated in my opinion

This comment has been minimized.

Copy link
@laviro

laviro Mar 17, 2019

Collaborator

got it.. thanks

return (
<div className={`datepicker ${className}`}>
{(() => {
switch (typeOfDateInput) {

This comment has been minimized.

Copy link
@laviro

laviro Mar 3, 2019

Collaborator

could be moved to an helper file into a function like: getDateViewByType ?

This comment has been minimized.

Copy link
@MariaAga

MariaAga Mar 7, 2019

Author Contributor

I moved it into a function. I think it should stay in the DateInput file and shouldn't be in a helper file because only the DateInput component will use it.

padding-bottom: 4px;
padding-top: 4px;
}
}

This comment has been minimized.

Copy link
@laviro

laviro Mar 3, 2019

Collaborator

I saw that there are small places where the elements are a bit off position, but I guess that the designers will have more to say about it than myself:

Current implementation:

date-picker

Original Design:

date-picker-design

Copy link
Contributor

amirfefer left a comment

Thanks @MariaAga , looks awesome ! 👍

Could you add an option to insert html attributes (like name which we need for forms) to date-time input?

@amirfefer

This comment has been minimized.

Copy link
Contributor

amirfefer commented Mar 7, 2019

one more thing, can you hide the popover when an outside click occurs ?

@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch 2 times, most recently from c212ecf to 5c959f9 Mar 7, 2019
@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented Mar 7, 2019

@amirfefer added inputProps to the datetimepicker input, and now the popover gets hidden when an outside click occurs

@dlabaj dlabaj self-requested a review Mar 11, 2019
@@ -0,0 +1,33 @@
.input-group.date-time-picker-pf.input-group {
width: 41.66666667%;

This comment has been minimized.

Copy link
@amirfefer

amirfefer Mar 18, 2019

Contributor

is it needed?
it cause incompatibility with other inputs in a form

IMHO the consumer should determine the width

Copy link
Contributor

amirfefer left a comment

css-datetime

As you can see, the top and left of that datepicker are not general, so the consumer would need to adjust it and overwrite some css
could you make the popover's margins referring the input's position ?

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Mar 19, 2019

@MariaAga I am not seeing a preview link. Can you generate one so we can perform a UX review? Thanks.

@laviro

This comment has been minimized.

Copy link
Collaborator

laviro commented Mar 19, 2019

@MariaAga This is how to deploy manually:

  1. yarn build:storybook && cp -r .out .public/patternfly-3

  2. yarn build:prdocs && cp -r packages/patternfly-4/react-docs/public .public/patternfly-4 && cp -r packages/patternfly-4/react-docs/public/assets .public/assets

  3. yarn run surge --project .public --domain $DEPLOY_DOMAIN

you may need to register into surge before.

@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented Mar 21, 2019

Thanks Ron, added a preview
@mcarrano

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Mar 21, 2019

This looks great @MariaAga . The only design thing I see is the positioning of the Today link. Can this be centered below the calendar? I did notice that you used the European style calendar with the week starting on Monday rather than Sunday like the core implementation. Is that something we can set as a Prop so the user can decide what calendar style to use?

I also felt it would be useful to include variants that either only have the date picker or the time picker by themselves.

@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch from 5c959f9 to 344f1cd Mar 24, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Apr 17, 2019

Thanks for making the changes @MariaAga . I updated to 'UX Approved'

@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented Apr 29, 2019

Split this PR into 2 - one for the date picker, and one for the time picker
#1875
#1873

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented May 1, 2019

Why the split? Should this PR be closed?

@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented May 1, 2019

Split for easier review of the code.
I would like to leave this PR open as this one includes the combination of the time picker and the date picker in the date time picker component

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented May 1, 2019

Should this then depend on the other two merging first?

@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented May 1, 2019

Yes

@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch from 08ab016 to bed8687 Jun 2, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 2, 2019

@waldenraines

This comment has been minimized.

Copy link
Collaborator

waldenraines commented Aug 15, 2019

@jeff-phillips-18 @tlabaj can someone review this please?

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Aug 15, 2019

I believe this depends on #1873 , then some rework.

@ares ares mentioned this pull request Sep 9, 2019
@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch from bed8687 to e732b69 Sep 15, 2019
@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented Sep 15, 2019

Rebased

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 15, 2019

PatternFly-React preview: https://patternfly-react-pr-1420.surge.sh

Copy link
Contributor

tlabaj left a comment

looks like you need to update your snapshots

Copy link
Collaborator

laviro left a comment

Great work @MariaAga, just waiting on that snapshots to be fixed :)

@MariaAga MariaAga force-pushed the MariaAga:feat/DataTimePicker branch from e732b69 to 9507afe Sep 19, 2019
@MariaAga

This comment has been minimized.

Copy link
Contributor Author

MariaAga commented Sep 19, 2019

Updated the snapshot, thanks!

@laviro
laviro approved these changes Sep 19, 2019
@jeff-phillips-18 jeff-phillips-18 merged commit 69107e1 into patternfly:master Sep 19, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 19, 2019

Your changes have been released in:

  • patternfly-react-extensions@2.20.5
  • patternfly-react@2.39.0
  • @patternfly/react-console@1.12.9

Thanks for your contribution! 🎉

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