Add Tooltip layer #1541
Add Tooltip layer #1541
Conversation
jaredoconnell
left a comment
jaredoconnell
left a comment
There was a problem hiding this comment.
I'm glad to see a layer system being added. I haven't reviewed this enough to approve, but I can confirm that it's working.
| `layer_type` and `fallback_widget` both represent the same content: | ||
|
|
||
| - `layer_type` is an enum which represents the layer's "semantic" content, with variants for common layer types. | ||
| - `fallback_widget` represents a widget which should be drawn at the root of the new layer. |
There was a problem hiding this comment.
I think the fallback vs built-in behavior is a little unclear in the documentation. Can you clarify it a bit?
There was a problem hiding this comment.
The next paragraph feels informative enough to me:
These two values are sent to the Masonry driver running the app; if the driver has built-in behavior for the given
layer_type, this behavior will be used.
Otherwise, the driver will add a new layer to the current [RenderRoot] withfallback_widgetas its root.
Do you think something is missing? Or do you think having these two items before the paragraph is confusing?
xStrom
left a comment
xStrom
left a comment
There was a problem hiding this comment.
The location of the tooltip should not be immediately under the cursor. Of course the cursor can be of various sizes, so it's not super trivial. However currently it is under even a small default cursor, so a small adjustment probably worth immediately to the tooltip position.
A robust tooltip system would also position the layer at various sides of the cursor depending on surrounding context, to not go off-screen etc. However, that's clearly a tricky piece of polish work for the future.
Also, good tooltips don't disappear on simple mouse move events, because not everyone has steady mouse control. There should be a trigger area, where if your movement delta is small enough within the area within a certain time period, you trigger the tooltip. Then the tooltip disappears after you leave the trigger area. However, this is again something that shouldn't block this PR and makes sense as future work.
Overall I think this is a good step forward and pending the few tiny tweaks I think can be merged.
PoignardAzur
force-pushed
the
layers_v2
branch
from
5c4420f to
9931a22
Compare
|
Made some adjustments to account for reviews. Will merge this next week unless someone objects. |
3 participants
See #1343.