carbon-addons-iot-react icon indicating copy to clipboard operation
carbon-addons-iot-react copied to clipboard

fix(datetimepickerv2): add single select and replace range select with new timepickerdropdown

Open jessieyan opened this issue 1 year ago • 15 comments

Closes #3503

Summary

  • added single select
  • replaced <TimePickerSpinner> with < TimePickerDropDown> for absolute range select

Change List (commits, features, bugs, etc)

  • items_here

Acceptance Test (how to verify the PR)

Single select image

Range select image

Regression Test (how to make sure this PR doesn't break old functionality)

  • tests here

Things to look for during review

  • [ ] Make sure all references to iot or bx class prefix is using the prefix variable
  • [ ] (React) All major areas have a data-testid attribute. New test ids should have test written to ensure they are not changed or removed.
  • [ ] UI should be checked in RTL mode to ensure the proper handling of layout and text.
  • [ ] All strings should be translatable.
  • [ ] The code should pass a11y scans (The storybook a11y knob should show no violations). New components should have a11y test written.
  • [ ] Unit test should be written and should have a coverage of 90% or higher in all areas.
  • [ ] All components should be passing visual regression test. For new styles or components either a visual regression test should be written for all permutations or the base image updated.
  • [ ] Changes or new components should either write new or update existing documentation.
  • [ ] PR should link and close out an existing issue

jessieyan avatar Aug 23 '22 15:08 jessieyan

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

Name Link
Latest commit 099a0bce1aa37b0476f8a9865bdf1536b052ed3e
Latest deploy log https://app.netlify.com/sites/carbon-addons-iot-react/deploys/631f6622986b480008ddee76
Deploy Preview https://deploy-preview-3523--carbon-addons-iot-react.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Aug 23 '22 15:08 netlify[bot]

Deploy Preview for ai-apps-pal-angular ready!

Name Link
Latest commit 099a0bce1aa37b0476f8a9865bdf1536b052ed3e
Latest deploy log https://app.netlify.com/sites/ai-apps-pal-angular/deploys/631f66222874a80009497a8c
Deploy Preview https://deploy-preview-3523--ai-apps-pal-angular.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Aug 23 '22 16:08 netlify[bot]

I try to open the time picker spinner dropdown and it does not open on this story.

davidicus avatar Aug 23 '22 19:08 davidicus

2. below

Tested on Mac Firefox

  1. When I focus on the time picker and enter a number, focus is lost
  2. I see "Invalid date" fairly often. a. It should give a standard invalid warning (red text below the input) if it's invalid, but still show what I selected b. It appears seemingly for no reason (Hitting Apply) Screen Shot 2022-08-23 at 3 47 34 PM

For 2, I will update the story to expose the warning text. image

jessieyan avatar Aug 23 '22 19:08 jessieyan

Tested on Mac Firefox

  1. When I focus on the time picker and enter a number, focus is lost
  2. I see "Invalid date" fairly often. a. It should give a standard invalid warning (red text below the input) if it's invalid, but still show what I selected b. It appears seemingly for no reason (Hitting Apply) Screen Shot 2022-08-23 at 3 47 34 PM

For 1, it's some logic from the existing <DateTimePickerV2> . Will take another look.

jessieyan avatar Aug 23 '22 20:08 jessieyan

Tested on Mac Firefox

  1. When I focus on the time picker and enter a number, focus is lost
  2. I see "Invalid date" fairly often. a. It should give a standard invalid warning (red text below the input) if it's invalid, but still show what I selected b. It appears seemingly for no reason (Hitting Apply) Screen Shot 2022-08-23 at 3 47 34 PM

For 1, it's some logic from the existing . Will take another look.

This is what I'd expect for 2A: Screen Shot 2022-08-23 at 4 09 58 PM

JordanWSmith15 avatar Aug 23 '22 20:08 JordanWSmith15

I try to open the time picker spinner dropdown and it does not open on this story.

Put zIndex setting in the story. Now it should work.

jessieyan avatar Aug 25 '22 18:08 jessieyan

Tested on Mac Firefox

  1. When I focus on the time picker and enter a number, focus is lost
  2. I see "Invalid date" fairly often. a. It should give a standard invalid warning (red text below the input) if it's invalid, but still show what I selected b. It appears seemingly for no reason (Hitting Apply) Screen Shot 2022-08-23 at 3 47 34 PM

For 1, it's some logic from the existing . Will take another look.

This is what I'd expect for 2A: Screen Shot 2022-08-23 at 4 09 58 PM

  1. The focus should be fixed
  2. The invalid text should now show under the start/end time input.

jessieyan avatar Aug 25 '22 18:08 jessieyan

The enter key problem should be fixed.

jessieyan avatar Aug 25 '22 20:08 jessieyan

I'm fixing the firefox browser issue that Jordan found. I need to format the hh:mm A date without using dayjs.

jessieyan avatar Aug 26 '22 16:08 jessieyan

Updated some formats to make sure it works with Firefox and other browsers.

jessieyan avatar Aug 26 '22 23:08 jessieyan

Two more things and then I think we're close:

  1. I need to click the "Apply" primary button twice before the dialog closes
  2. Null values don't seem to be demonstrated. A. What does a null value look like, exactly? I would expect that placeholder grey text would show the browser's locale format: Screen Shot 2022-08-29 at 8 15 38 AM B. There is no way to clear the field back to null. I think we should just add a button here, for now, that closes the flyout and nulls out the field to the above state.
Screen Shot 2022-08-29 at 8 03 53 AM

@JordanWSmith15

  1. It works for me in both chrome and firefox. Anything special in your test?
  2. From what I saw, if giving default value, the default state is alway with the default value. If no default value, it will go to preset looks like this. image

So it's either default value state or updated value state. @davidicus Hi David, can you also confirm please?

jessieyan avatar Aug 29 '22 13:08 jessieyan

@jessieyan

  1. Maybe it's specific to Mac? Do you have a PC? Note that you must change the currently selected date, THEN try to hit "Apply" to produce the issue.
  2. I'm talking specifically about being able to clear the "Single select" version's value. Graphite will not be using the Relative version for this release, so I am very focused on this component: https://deploy-preview-3523--carbon-addons-iot-react.netlify.app/?path=/story/2-watson-iot-experimental-%E2%98%A2%EF%B8%8F-datetimepickerv2--single-select

JordanWSmith15 avatar Aug 29 '22 14:08 JordanWSmith15

Added clear button to reset date and time value in single select.

jessieyan avatar Aug 30 '22 20:08 jessieyan

@jessieyan A few items:

  • It looks like you took away the TimePickerSpinner option. The new input options were supposed to be put behind a prop. This component should work exactly as it did before unless this prop is provided.
  • Test for the old 24hour format should remain to make sure the old functionality still works. We should write additional test to test this new TimePickerDropdown functionality.
  • When you are going from a relative option with an existing value to the absolute value the existing value is being turned to "invalid date to invalid date". This remains even when you go back to the relative option.
  • When you go to use the absolute value option on any story the input states "invalid date to invalid date" and the inputs are in the error state. This should not occur unless the user inputs an invalid date.
  • I am not seeing a story knob for switching between single and range or to use the old TImePickerSpinner vs. the new TimePickerDropdown
  • When I click enter on the TimePickerDropdown component, inside DateTimePicker, I get an error thrown image

davidicus avatar Sep 01 '22 19:09 davidicus

@EdmondCShaffer @herleraja Hey guys, we want to the replace <TimePickerSpinner> with < TimePickerDropDown> in <DateTimePickerV2>. We need your feedback if you would like the keep <TimePickerSpinner> or using the new < TimePickerDropDown>.

The input and output of <DateTimePickerV2> will stay the same. Main differences are <TimePickerDropDown> uses a list spinner for hours, minutes and AM/PM. Also <TimePickerDropDown> uses a 12 hr format for displaying the time.

Please let us know your preference and concerns.

jessieyan avatar Sep 02 '22 14:09 jessieyan

DateTimePickerV2

He @jessieyan I think its fine. Only to note that we use the 24-hour format in the dashboard card DateTimePickerV2. I think by default its 24 hours then we don't have much concern here.

Btw what is the difference between DateTimePickerV2 and DateTimePicker?

herleraja avatar Sep 05 '22 07:09 herleraja

@davidicus I renamed old <DateTimePickerV2> to < DateTimePickerV2WithoutTimeSpinner> and use <DateTimePickerV2> to wrap < DateTimePickerV2WithoutTimeSpinner> and < DateTimePickerV2WithTimeSpinner>. I saw the PublicApi show some red lines. Should I define all props in the wrapper <DateTimePickerV2> as well?

jessieyan avatar Sep 06 '22 13:09 jessieyan

@davidicus I renamed old <DateTimePickerV2> to < DateTimePickerV2WithoutTimeSpinner> and use <DateTimePickerV2> to wrap < DateTimePickerV2WithoutTimeSpinner> and < DateTimePickerV2WithTimeSpinner>. I saw the PublicApi show some red lines. Should I define all props in the wrapper <DateTimePickerV2> as well?

Fixed the the propTypes and defaultProps in <DateTimePickerV2>.

jessieyan avatar Sep 06 '22 17:09 jessieyan

@jessieyan A few items:

  • It looks like you took away the TimePickerSpinner option. The new input options were supposed to be put behind a prop. This component should work exactly as it did before unless this prop is provided.
  • Test for the old 24hour format should remain to make sure the old functionality still works. We should write additional test to test this new TimePickerDropdown functionality.
  • When you are going from a relative option with an existing value to the absolute value the existing value is being turned to "invalid date to invalid date". This remains even when you go back to the relative option.
  • When you go to use the absolute value option on any story the input states "invalid date to invalid date" and the inputs are in the error state. This should not occur unless the user inputs an invalid date.
  • I am not seeing a story knob for switching between single and range or to use the old TImePickerSpinner vs. the new TimePickerDropdown
  • When I click enter on the TimePickerDropdown component, inside DateTimePicker, I get an error thrown image

@davidicus This should be addressed by adding a new prop useNewTimeSpinner and the wrapper component <DateTimePickerV2> which determines whether to render <DateTimePickerV2WithTimeSpinner> or <DateTimePickerV2WithoutTimeSpinner>. Added knob of useNewTimeSpinner in Selected absolute story and add a separate story Range select with new time spinner.

image

jessieyan avatar Sep 06 '22 19:09 jessieyan

In this story you can see the tooltip value is showing the mask value instead of the value of the input as it does in our current version.

Next This PR

davidicus avatar Sep 09 '22 20:09 davidicus

This may not something that we need to fix now but in the input when you click a on a date from an empty state you get a time as seen in this image. Should this value be added as the value for the TimePickerDropdown? image

davidicus avatar Sep 09 '22 20:09 davidicus

In this story when you go to a preset value then chose a relative value then go to the absolute value calendar, there is a discrepancy in what is in the input field and what is shown in the TImePickerDropdown image

@davidicus Yeah, there is a bug here. This bug is not new though. When change time values in relative, they are not picked up by absolute. I will see if i can fix it. Otherwise, I think we could open an issue to track it.

davidicus avatar Sep 09 '22 20:09 davidicus

this

In this story you can see the tooltip value is showing the mask value instead of the value of the input as it does in our current version.

Next This PR

This is the original design I think. The design before is if the whole value could not be displayed, then show it in tooltip. If the whole value could be shown, then show mask value in the tooltip.

jessieyan avatar Sep 12 '22 03:09 jessieyan

This may not something that we need to fix now but in the input when you click a on a date from an empty state you get a time as seen in this image. Should this value be added as the value for the TimePickerDropdown? image

@davidicus The behavior I saw from the code is if there is no time value provided, we set it to 00:00 as default. That's why it is 12:00 AM. We can open an issue to track the default behavior.

jessieyan avatar Sep 12 '22 03:09 jessieyan

@jessieyan there may be something in the code that does not allow for this check to be dynamic but when I make the input smaller I am not getting the value in the tool tip. Can we check if this is the case and what we can do about it. I think this would be the last thing and then we can get this merged in. Thanks for working through this. I know its a complicated component. image

davidicus avatar Sep 12 '22 14:09 davidicus

@jessieyan there may be something in the code that does not allow for this check to be dynamic but when I make the input smaller I am not getting the value in the tool tip. Can we check if this is the case and what we can do about it. I think this would be the last thing and then we can get this merged in. Thanks for working through this. I know its a complicated component. image

@davidicus It is actually a bug to fix. I've fixed it. The tooltip should show the actual value (humanValue). If humanValue not exists, then use dateTimeMask.

jessieyan avatar Sep 12 '22 17:09 jessieyan

In this story when you go to a preset value then chose a relative value then go to the absolute value calendar, there is a discrepancy in what is in the input field and what is shown in the TImePickerDropdown image

@davidicus Yeah, there is a bug here. This bug is not new though. When change time values in relative, they are not picked up by absolute. I will see if i can fix it. Otherwise, I think we could open an issue to track it.

Hi @davidicus , I opened issue 3547 to track the issue that values are not updated when switching between Relative and Absolute.

jessieyan avatar Sep 12 '22 18:09 jessieyan