carbon-addons-iot-react
carbon-addons-iot-react copied to clipboard
fix(datetimepickerv2): add single select and replace range select with new timepickerdropdown
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
Range select
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
orbx
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
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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
I try to open the time picker spinner dropdown and it does not open on this story.
2. below
Tested on Mac Firefox
- When I focus on the time picker and enter a number, focus is lost
- 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)
For 2, I will update the story to expose the warning text.
Tested on Mac Firefox
- When I focus on the time picker and enter a number, focus is lost
- 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)
For 1, it's some logic from the existing <DateTimePickerV2> . Will take another look.
Tested on Mac Firefox
- When I focus on the time picker and enter a number, focus is lost
- 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)
For 1, it's some logic from the existing . Will take another look.
This is what I'd expect for 2A:
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.
Tested on Mac Firefox
- When I focus on the time picker and enter a number, focus is lost
- 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)
For 1, it's some logic from the existing . Will take another look.
This is what I'd expect for 2A:
- The focus should be fixed
- The invalid text should now show under the start/end time input.
The enter key problem should be fixed.
I'm fixing the firefox browser issue that Jordan found. I need to format the hh:mm A date without using dayjs.
Updated some formats to make sure it works with Firefox and other browsers.
Two more things and then I think we're close:
- I need to click the "Apply" primary button twice before the dialog closes
- 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: 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.
@JordanWSmith15
- It works for me in both chrome and firefox. Anything special in your test?
- 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.
So it's either default value state or updated value state. @davidicus Hi David, can you also confirm please?
@jessieyan
- 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.
- 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
Added clear button to reset date and time value in single select.
@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 newTimePickerDropdown
- When I click enter on the TimePickerDropdown component, inside DateTimePicker, I get an error thrown
@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.
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?
@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?
@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 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 newTimePickerDropdown
- When I click enter on the TimePickerDropdown component, inside DateTimePicker, I get an error thrown
@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.
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 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?
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
@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.
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.
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?
@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 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.
@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.
@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
.
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
@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.