DropdownInput preview support and ColorButton history improvements #1598
Conversation
editor/src/messages/portfolio/document/node_graph/node_graph_message_handler/node_properties.rs
Outdated
Show resolved
Hide resolved
|
sry, it's still work in progress, not ready even for reviewing. I should open this as a draft pr... |
|
Ah, yes, good to open PRs early but just please mark them as a draft. I usually don't poke around draft PRs. But just make sure to ping me once you're ready for review, and switch it to non-draft. Thanks :) |
|
@Keavon Actually, I opened this pr actually would like to confirm two user experience behaviors first.
Does it mean user type in TextInput in right panel, would like the text change according to in the canvas as user typing.(Right now it only changes when the TextInput is unfocus.)
Does it means these sliders? (Right now these sliders do not save any history steps separately, only save a history step after close Color Panel) |
If I'm interpreting your question correctly, I think you're asking if text created with the Text tool should be re-rendered as the user types in the Properties panel? My answer to that is: maybe, but only if the performance isn't too bad. There is also #1589 currently open so let's avoid implementing that right now in case it causes conflicts. I'm mostly just interested in having the text widget send its real-time text but letting the user of the text, like the Properties panel for the Text node, decide if it wants to use the real time version or the committed version (which is what is used right now).
All of the above should send the history update as described, but duplicates should be avoided if the user committed a change that resulted in a value that's the same as when they started to edit that value. |
Keavon
force-pushed
the
ux-improvement
branch
from
ef4d6c4
to
7c6334e
Compare
zhiyuang
changed the title
|
@Keavon could have a code review when you have time. This PR is mainly about Dropdown preview ability and Color Button related history actions (All actions in below image). For TextInput widget, I haven't handled in this PR. I found unless we really need send updates on every typing by the users, current text widget has satisfied enough right now. Maybe we could return to this if we meet some concrete issues in future 🤔 |
Keavon
force-pushed
the
ux-improvement
branch
from
65f6f9b
to
fd2c3df
Compare
Keavon
changed the title
Keavon
force-pushed
the
ux-improvement
branch
from
fd2c3df
to
8c49358
Compare
| @@ -67,6 +67,10 @@ pub struct DropdownInput { | |||
|
|
|||
| pub tooltip: String, | |||
|
|
|||
| #[derivative(Default(value = "false"))] | |||
| #[serde(rename = "previewOnHover")] | |||
| pub preview_on_hover: bool, | |||
There was a problem hiding this comment.
I'm pretty sure this shouldn't be necessary. It can work just like your previous PR where NumberInput was set to send both a preview value and a committed value. Then the backend code which wants this widget to send preview values would just handle those temporary values directly from the backend, whereas those which only care about the final committed value do nothing with the preview values. The approach I'm suggesting here would allow this widget to have a more standardized API alongside NumberInput widgets and others, and would be generally cleaner.
frontend/wasm/src/editor_api.rs
Outdated
| /// Update the value of a given UI widget, and commit it to the history | ||
| #[wasm_bindgen(js_name = widgetValueRevert)] | ||
| pub fn widget_value_revert(&self) -> Result<(), JsValue> { | ||
| self.dispatch(DocumentMessage::Undo); |
There was a problem hiding this comment.
This makes me nervous. But it shouldn't be necessary anymore once you make the change I suggested above.
|
I'm marking this as a draft while awaiting the requested changes. Please un-mark as draft and also ping me when ready for a second review. Thanks! |
|
Hi, I'm just checking up on how things are going with this feedback round. Thanks :) |
|
sry for the late, a little busy these two weeks. I'll look forward to see if I could find some time next week. |
|
Sure thing, thank you for the update. |
|
!build |
|
!build |
|
editor/src/messages/portfolio/document/document_message_handler.rs
Outdated
Show resolved
Hide resolved
|
!build |
|
I rebased this for you, and now strangely I'm getting fatal console errors when I hover over the dropdown menu entries. Can you please help investigate? I committed some debug instrumentation which points out that it's getting a value of -1 where it should be able to fit as a |
|
zhiyuang
force-pushed
the
ux-improvement
branch
from
b200c33
to
d6e357f
Compare
|
it's related to these codes, if I comment these out, it could work. d6e357f |
|
but I still don't know how fix this. I'm not familiar with MenuList yet. It should be related to this PR: #1499 |
|
Ah, that's a good hint, thank you for investigating and narrowing it down to that! If you'd like me to take it from here, I can do that in the next couple days. Or if you'd like to keep digging and find the solution, please feel free to do that as well. |
| } | ||
| // if (interactive && newHighlight?.value !== activeEntry?.value && newHighlight) { | ||
| // dispatch("activeEntry", newHighlight); | ||
| // } |
There was a problem hiding this comment.
for the preview case for blend mode. The activeEntry actually is the one being hovered, not the one being highlighted. So in this case it could be different. Maybe @0HyperCube could help have a look here.
|
@Keavon updated, pls check |
Keavon
force-pushed
the
master
branch
2 times, most recently
from
7bf7f92
to
5f4960d
Compare
|
Pardon the very long delay, this required rebuilding some mental context and I was quite busy with things unrelated to this. Thank you for helping make this happen, and your patience in my finally returning to merge it. |
As follow ups of #1584 (comment)