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.

Sign up for GitHub

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

Jump to bottom

NG1691 - Timepicker fix on custom format #8841

Merged
merged 2 commits into from
Jun 27, 2024
Merged

NG1691 - Timepicker fix on custom format #8841

merged 2 commits into from
Jun 27, 2024

Conversation

yohannahbautista
Copy link
Contributor

Explain the details for making this change. What existing problem does the pull request solve?

Timepicker fix on custom format

Related github/jira issue (required):

Closes infor-design/enterprise-ng#1691

Steps necessary to review your pull request (required):

Included in this Pull Request:

  • A note to the change log.

@yohannahbautista yohannahbautista requested a review from a team as a code owner June 21, 2024 03:15
glenlieorillo
glenlieorillo suggested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@glenlieorillo glenlieorillo left a comment

Choose a reason for hiding this comment

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

LTR

  • Working as expected

image

image

image

RTL

  • Displaying Invalid time message

Steps to reproduce:

  1. Go to https://ng1691-timepicker-enterprise.demo.design.infor.com/components/timepicker/test-custom-format.html?locale=ar-SA
  2. Click the clock icon
  3. Choose time from the dropdown
  4. Click Set time
  5. See error
Screen.Recording.2024-06-21.at.2.26.32.PM.mov

image

image

jbrcna
jbrcna approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@jbrcna jbrcna left a comment

Choose a reason for hiding this comment

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

image

same observation with @glenlieorillo on RTL
image

@yohannahbautista
Copy link
Contributor Author

@jbrcna @glenlieorillo
https://main-enterprise.demo.design.infor.com/components/timepicker/example-index.html?locale=ar-SA
This looks to be a separate bug that's not connected to the enhancement, so could this be reported separately instead?

tmcconechy
tmcconechy approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

Looks good. I agree lets make a new issue for the other point

@tmcconechy tmcconechy added the ready for qa Ready for QA to review label Jun 26, 2024
jbrcna
jbrcna approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@jbrcna jbrcna left a comment

Choose a reason for hiding this comment

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

image

glenlieorillo
glenlieorillo approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@glenlieorillo glenlieorillo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

image

Created an issue for the RTL bug: #8857

cc: @tmcconechy

@tmcconechy tmcconechy merged commit 18bba22 into main Jun 27, 2024
2 checks passed
@tmcconechy tmcconechy deleted the NG1691-timepicker branch June 27, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmcconechy tmcconechy tmcconechy approved these changes

@jbrcna jbrcna jbrcna approved these changes

@glenlieorillo glenlieorillo glenlieorillo approved these changes

Assignees
No one assigned
Labels
ready for qa Ready for QA to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timepicker: Add Support for HHmmss Format
4 participants
@yohannahbautista @tmcconechy @jbrcna @glenlieorillo