Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Hide eye icon on error #1819

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Hide eye icon on error #1819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
614dc13
Hide eye icon on error
s9ferech Aug 26, 2021
c3f8093
Update CHANGELOG.md
s9ferech Aug 26, 2021
f44339b
Merge branch 'develop' into wip/fr/hide-eye
MichaelMauderer Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,22 @@
enhancements and fixes made to the Enso compiler, you can find their release
notes [here](https://github.com/enso-org/enso/blob/main/RELEASES.md).

<br/>![Bug Fixes](/docs/assets/tags/bug_fixes.svg)

#### Visual Environment

- ["Show visualization" icon is hidden on errors.][1819] Nodes usually reveal an
eye shaped button on hover that can be used to show or hide the node's
visualization. This button will not be visible when the node is in an error
state where no visualization would be available.

<br/>

[1801]: https://github.com/enso-org/ide/pull/1801
[1775]: https://github.com/enso-org/ide/pull/1775
[1798]: https://github.com/enso-org/ide/pull/1798
[1804]: https://github.com/enso-org/ide/pull/1804
[1819]: https://github.com/enso-org/ide/pull/1819

# Enso 2.0.0-alpha.11 (2021-08-09)

Expand Down
24 changes: 14 additions & 10 deletions src/rust/ide/view/graph-editor/src/component/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,16 +675,6 @@ impl Node {
});


// === Action Bar ===

let visualization_enabled = action_bar.action_visibility.clone_ref();
out.source.skip <+ action_bar.action_skip;
out.source.freeze <+ action_bar.action_freeze;
show_action_bar <- out.hover && frp.show_quick_action_bar_on_hover;
eval show_action_bar ((t) action_bar.set_visibility(t));
eval frp.show_quick_action_bar_on_hover((value) action_bar.show_on_hover(value));


// === View Mode ===

model.input.set_view_mode <+ frp.set_view_mode;
Expand Down Expand Up @@ -746,6 +736,7 @@ impl Node {
preview_visible <- preview_visible && has_expression;
preview_visible <- preview_visible.on_change();

let visualization_enabled = action_bar.action_visibility.clone_ref();
visualization_visible <- visualization_enabled || preview_visible;
visualization_visible <- visualization_visible && no_error_set;
visualization_visible_on_change <- visualization_visible.on_change();
Expand Down Expand Up @@ -779,6 +770,19 @@ impl Node {

}


// === Action Bar ===

frp::extend! { network
out.source.skip <+ action_bar.action_skip;
out.source.freeze <+ action_bar.action_freeze;
frp.show_quick_action_bar_on_hover <+ no_error_set;
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be more tricky than this solution. We don't necessarily want to hide ALL quick action icons. While there is just one at the moment, there will be others, and they might be independent of the error state of the node. So hiding all of them would not be what is needed. Instead, we just want to hide the visibility action icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should happen with the other icons then? Should they move to the left to take the place of the eye icon, or should they stay where they are?

Copy link
Contributor

Choose a reason for hiding this comment

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

They should move to the left. If I do remember correctly, this might require some refactoring of the logic, though, as the positions might be fixed at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Moving icons is bad solution IMO. The eye icon can be grayed out instead. Changing controls placement is super confusing to users. Peopel will get used to the fact that first icon is visualization. Imagine a situation when you try to click it, but then error occurs and you click other icon because it just changed place - this is a very bad UX

Copy link
Contributor

Choose a reason for hiding this comment

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

@wdanilo Then we need to come up with a proper visual design for this. Fading out would probably not work because it would be confusable with the current active/not-active semantics, unless we re-design those.

show_action_bar <- out.hover && frp.show_quick_action_bar_on_hover;
eval show_action_bar ((t) action_bar.set_visibility(t));
eval frp.show_quick_action_bar_on_hover ((value) action_bar.show_on_hover(value));
}


// === Profiling Indicator ===

frp::extend! { network
Expand Down