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(datetimepicker): new DateTimePicker component #1072

Merged
merged 17 commits into from
Apr 22, 2020

Conversation

enricoberti
Copy link
Contributor

@enricoberti enricoberti commented Apr 7, 2020

Closes #854 closes #1114

Summary

  • Supports presets, relative and absolute times
  • Presets and relative options are configurable

image

image

image

Change List (commits, features, bugs, etc)

  • Known item to be addressed after this PR is through: RTL support

Acceptance Test (how to verify the PR)

  • ?path=/story/watson-iot-experimental-datetime-picker--default and sibling stories

@netlify
Copy link

netlify bot commented Apr 7, 2020

Deploy preview for carbon-addons-iot-react ready!

Built with commit a6cde9a

https://deploy-preview-1072--carbon-addons-iot-react.netlify.app

@davidicus
Copy link
Collaborator

davidicus commented Apr 7, 2020

@enricoberti this is looking great! I did see one small bug. If you click on your start and end date for custom range. Then click on any date under the selected. It will start to increment the end date by months. Also if you click cancel and then try to go back in the calendar selections are still there and not cleared.

@JordanWSmith15
Copy link
Collaborator

JordanWSmith15 commented Apr 7, 2020

@enricoberti Looking great! A few scenarios to consider checking that will cause problems that Graphite has had to fix for the Dropdown component:

  1. When positioned near the bottom of a page, the editing dialog should expand upwards, rather than below the page fold. Example at bottom of this storybook: https://pages.github.ibm.com/maximo-app-framework/graphite/react/storybook/?path=/story/components-%E2%9C%93-dropdown--dropdown
  2. Plan for issues with z-index. It would be good to demonstrate what this does inside of a modal, for instance. The editing dialog of this component should overlap the modal buttons and expand outside of a modal, rather than cause an internal scroll: See dropdown 2: https://pages.github.ibm.com/maximo-app-framework/graphite/react/storybook/?path=/story/components-%E2%9C%93-dropdown--datasource-dropdown-on-a-dialog

@enricoberti
Copy link
Contributor Author

thanks @davidicus and @JordanWSmith15, valid feedback! fixing the issues now

Copy link
Contributor

@stuckless stuckless left a comment

Choose a reason for hiding this comment

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

This was very good. Lots of globalization issues to address.

I did notice a couple of things...

  1. when entering times there is no validation there... I can enter abcd and doesn't care.
  2. I did have a couple of dates that didn't fit in the from -> to header and it just bleeded into the icons... these need to be ellipsised
  3. I could not really figure out how the ranging worked. What I tried originally was just to click and drag to select a rang but that didn't work. Does the PAL talk about what the expectations are here for selecting ranges. I could never really figure out how to set the start and end. It seemed like one date was always fixed, so every click would create a range to the point or from the point. It could be the sample I was using, not sure.

src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
@enricoberti
Copy link
Contributor Author

thank you for the feedback @stuckless ! i pushed a few changes:

  • a bit more documentation on the returned object
  • i18n
  • better cancelation flow
  • date time mask
  • ellipsis on overflowing text
  • filter out non HH:mm characters on the timepickerspinner

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

In addition to these comments,

  • I don't see a new snapshot being added for this component. Even though it's under experimental, I think we still generate snaps for those components?
  • The component needs to be added to the central entrypoint index.js as well
    • The publicAPI snapshot should update to include the new component
    • The storybook welcome story snapshot, the exports table should be modified to include the new export.

Comment on lines +121 to +144
/*
{
kind: String // the type of selection, one of PICKER_KINDS (PRESET, RELATIVE, ABSOLUTE)
preset: {
label: String // the label of the selected preset
offset: Number // the offset in minute
},
relative: {
start: Date // the start point in time
end: Date // the end point in time
lastNumber: Number // quantity of interval
lastInterval: String // one of INTERVAL_VALUES
relativeToWhen: String // one of RELATIVE_VALUES, indicates to what point in time the selection is relative to
relativeToTime: String // in the HH:MM format
},
absolute: {
start: Date // the start point in time
end: Date // the end point in time
startDate: String // start date in the mask or default format
startTime: String // in the HH:MM format
endDate: // end date in the mask or default format
endTime: String // in the HH:MM format
},
} */
Copy link
Member

Choose a reason for hiding this comment

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

This documentation is prone to being outdated as the component changes. Instead, this object should be logged out in the action logger within storybook when the callback is called.

Copy link
Member

Choose a reason for hiding this comment

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

If it is already being logged out in the action panel, we can delete that code comment section

src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.story.jsx Outdated Show resolved Hide resolved
src/components/DateTimePicker/_date-time-picker.scss Outdated Show resolved Hide resolved
src/components/DateTimePicker/DateTimePicker.jsx Outdated Show resolved Hide resolved
@davidicus
Copy link
Collaborator

Minor RTL issue when picking a custom range

image

transition: background-color $duration--fast-01 carbon--motion(standard);
.#{$iot-prefix}--date-time-picker__field {
background: none;
-webkit-appearance: none;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that were are using vender prefixes here and there are few places were we should be using tokens and rems. I have opened a new issue to track this as to not hold up this component. #1114

@davidicus davidicus dismissed stuckless’s stale review April 22, 2020 20:23

Enrico has pushed requested changes.

@davidicus davidicus merged commit b3d81b7 into master Apr 22, 2020
@davidicus davidicus deleted the enrico-854-datetimepicker branch April 22, 2020 20:52
@tay1orjones
Copy link
Member

🎉 This PR is included in version 2.70.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DateTimePicker] Fix issues with sass partial [datepicker] Time range / date-time picking for dashboards
5 participants