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(ui5-datepicker): implement valuestatemessage slot #1476

Merged
merged 16 commits into from
May 5, 2020

Conversation

fifoosid
Copy link
Contributor

🎉

@fifoosid
Copy link
Contributor Author

Fixes #1385

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Two comments:

  • If the valueStateMessage content/text is changed when it is opened, the change would not be applied as we display a copy/clone, but it`s rather unusual, in most cases it would be fine as whenever the input field is clicked, the component is being invalidated and the valueStateMsg is up to date. @vladitasev what do you think, should we observe the original.

  • when I click the toggle icon, the value state msg is shown for a second and then the calendar is displayed as usual, I did not check how it works in openui5 (might be the same), but it looks strange, almost like a bug, because something blinks for a moment and it`s gone.

@fifoosid
Copy link
Contributor Author

The second comment is fixed, and the first one I would rather not implement until someone needs it.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Two comments:

  • in phone mode the Input with suggestions and value state msg throws error, because the inputDomRef is not there ( in phone mode)
  • should we displayed the value state msg of DatePicker within the dialog in phone mode as well?

packages/main/src/DatePicker.js Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

With the current changes in the Input.js the following stops working: previously the value state message used to be displayed in the dialog on phone, now - it is not. If for the DatePicker that is fine, I think the Input should show the message within the dialog, at least it was intentionally implemented in this way.

packages/main/src/Input.js Outdated Show resolved Hide resolved
packages/main/src/Input.js Show resolved Hide resolved
Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

The link inside does not work, when you click it, it does not change the URL, but closes the popup instead.

packages/main/test/pages/DatePicker.html Outdated Show resolved Hide resolved
@ilhan007 ilhan007 dismissed their stale review May 5, 2020 10:38

tested now works fine

@fifoosid fifoosid merged commit 82b3d41 into master May 5, 2020
@fifoosid fifoosid deleted the datepicker-valuestatemessage branch May 5, 2020 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants