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-date-picker): introduce value-state-change event #8133

Merged
merged 10 commits into from
Jan 26, 2024

Conversation

hinzzx
Copy link
Contributor

@hinzzx hinzzx commented Jan 18, 2024

Currently in our <ui5-date-picker> component we have internal validation regarding value-state, which was setting up ValueState.Error/ValueState.None whenever the value in the component was invalid/valid.

The problem here is that when an app developer sets the value-state property explicitly it was being overwritten by the control, which is not always the expected behaviour.

To address this problem we are introducing the value-state-change event, which is fired before the internal validation/invalidation of the component. It is preventable, so when the app developer wants to take care of the state management of the component, they just prevent the event.

Behaviour with event prevented, and explicitly set valueState property to Error:

2024-01-18_09-44-04


Behaviour without event being prevented, and explicitly set valueState property to Error:

2024-01-18_09-45-14

Fixes: #8005

@ilhan007
Copy link
Member

related to: #7444

@ilhan007
Copy link
Member

Everything good so far, the only thing I would like to discuss briefly next week is if we have to rather update the this.valueState before firing the event and later revert its value if the event is prevented. Or, to keep it like it is. I think in other components we have done the first, but still worth discussing it briefly.

@ilhan007
Copy link
Member

Hi colleagues, @hinzzx as I mentioned previously, I wanted to discuss one detail before enforcing it as a review comment.
After brief discussion, we think it's more consistent to update the valueState property before firing the event, so that accessing this.valueState in the handler would return the new/updated value of the prop. And, afterwards, if the event turns out to be prevented - we revert the valueState prop to its previous value.

@hinzzx hinzzx requested a review from ilhan007 January 23, 2024 09:47
@ilhan007
Copy link
Member

Thank you! I don't have any further comments, I will leave to @tsanislavgatev to also review/test/approve/merge the change.

packages/main/src/DatePicker.ts Show resolved Hide resolved
packages/main/src/DatePicker.ts Show resolved Hide resolved
@tsanislavgatev tsanislavgatev merged commit 69143b0 into main Jan 26, 2024
8 checks passed
@tsanislavgatev tsanislavgatev deleted the feat-dp-vs-chng-evt branch January 26, 2024 11:48
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.

[DatePicker]: Invalid valueState on focus out of empty input
4 participants