Skip to content

fix(carbon): fix time picker initial value #1157

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

Conversation

GilbertCherrie
Copy link
Contributor

Fixes: #1155

Description:
Fixed the AM/PM select initial value for the time picker component.

@DataDrivenFormsBot
Copy link

A new version (fix) will be released: v3.15.4 [DataDrivenFormsBot]

@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #1157 (bf8d30e) into master (03df194) will decrease coverage by 0.01%.
The diff coverage is 95.45%.

❗ Current head bf8d30e differs from pull request most recent head 6abea25. Consider uploading reports for the commit 6abea25 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
- Coverage   94.89%   94.88%   -0.02%     
==========================================
  Files         209      209              
  Lines        3567     3579      +12     
  Branches     1234     1241       +7     
==========================================
+ Hits         3385     3396      +11     
- Misses        182      183       +1     
Impacted Files Coverage Δ
...nt-mapper/src/time-picker-date/time-picker-date.js 100.00% <ø> (ø)
...nt-mapper/src/time-picker-base/time-picker-base.js 95.83% <95.45%> (-4.17%) ⬇️

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 f698d5a...6abea25. Read the comment docs.

Copy link
Contributor

@rvsia rvsia left a comment

Choose a reason for hiding this comment

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

Hello,

thank you for this PR.

however, several changes are required:

  1. time-picker-base file is just a styling component with no internal logic, to implement this fix we have to update time-picker-date.js component (string variant is already working well)

  2. initialLoad prop is not neccesary, we can just use initialValue (resp. input.value)

  3. for setting the correct value we can just update the format state variable in an init function

before

const [format, selectFormat] = useState('AM');

after

const [format, selectFormat] = useState(() => input.value?.getHours?.() >= 12 ? “PM” : “AM”);

@rvsia rvsia added bug Something isn't working Carbon PRs/Issues for IBM's Carbon design system labels Oct 25, 2021
@GilbertCherrie
Copy link
Contributor Author

@rvsia I tried out your suggestions and it does not seem to be working. If you would like to test it out I pushed the changes along with the test data I was using in the carbon component mapper sandbox. Not really sure why it is not working initially I was trying out something similar to what your suggestions mentioned but it didn't work which is why I ended up with the complicated code in my first commit.

@rvsia
Copy link
Contributor

rvsia commented Oct 26, 2021

closing in favor of #1160

@rvsia rvsia closed this Oct 26, 2021
@GilbertCherrie GilbertCherrie deleted the fix_time_picker_initial_value branch October 26, 2021 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Carbon PRs/Issues for IBM's Carbon design system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carbon Component Mapper Time Picker Initial Value Bug
3 participants