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

The commit behavior of the PropertyPane editor needs some modifications #915

Closed
Rock2000 opened this issue Jul 17, 2024 · 1 comment · Fixed by #928
Closed

The commit behavior of the PropertyPane editor needs some modifications #915

Rock2000 opened this issue Jul 17, 2024 · 1 comment · Fixed by #928
Assignees
Labels
bug Something isn't working presentation Issues that @itwin/itwinjs-core-presentation team is going to work on

Comments

@Rock2000
Copy link
Contributor

Describe the bug

When a commit is triggered from the UI, the component will propagate the change event even if the 'backing' value is exactly the same. This results in a transaction on the undo stack when a user simply enters edit mode and then clicks off (changes nothing). Also the ESC key seems to commit, which it should not.

To Reproduce

No response

Expected Behavior

The ESC key should be an easy one. If ESC is hit, the formatted original value should be restored to the widget, and I think edit mode of the property should be ended.

Fixing the other issue brings up a few more situations to worry about. For example, a backing value of 17.0026 could be displayed as 17.0. But it is possible that the user actually wants to edit the value and set it to exactly 17. So I think we do need to support that case also, but probably only via an explicit Enter key. Here is what I think the Booster team would like to see for behavior once in edit mode:

if (loseFocus) {
if (edit string != originalFormattedString)
tryCommit()
} else if (EnterKey)
tryCommit()
}

tryCommit() {
newValue = parse(editText)
if (isValid(newValue) && newValue != originalValue)
commitForReal()
}

Screenshots

No response

Desktop (please complete the applicable information)

iTwin.js 4.7.2
AppUI 4.12.0

Additional context

No response

@grigasp
Copy link
Member

grigasp commented Aug 7, 2024

The fix released with @itwin/[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working presentation Issues that @itwin/itwinjs-core-presentation team is going to work on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants