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

Improve styling and simplfy workflow example #160

Merged
merged 5 commits into from
Jan 17, 2022
Merged

Conversation

planger
Copy link
Member

@planger planger commented Jan 7, 2022

Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

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

Thanks Philip! I like the improved styling of the workflow example! 🎉
I spotted one potential issue with the dashed strokes, please see inline.

Comment on lines 88 to 90
stroke-width: 1.5px;
stroke-dashoffset: 5;
stroke-dasharray: 5, 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I like the visual differentiation for selection/mouseover via dashed stroke. But due to the dashed line, edges might flicker on mouseover (see also screenshot). Maybe we could think of a solid line in a contrast color?

glsp-example-dashed-flickering

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Nina, you are right! We typically fix those issues by adding a transparent wider edge above the visible edge, which helps selection the edge too.
However, we also want to stay very generic, so probably dashes aren't a good choice for hover feedback.
I replaced it with opacity for now.
What do you think?
Also feel free to suggest different colors for the nodes if you don't like them.

@planger planger requested a review from ndoschek January 11, 2022 17:14
Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

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

Thanks Philip! The selection using opacity looks great!

I played around with the colors a bit, and using complementary colors for the nodes would look great to me, e.g. some darker orange (e.g. #db8651) and some teal (e.g. #5b9fa8).
This would also have the benefit that the blue selection color would give bit more contrast to both colors imo.
Also, I would suggest to use var(--theia-focusBorder) instead of var(--theia-menu-selectionBackground) as selection color, as it a bit brighter in dark mode as well.

Proposal in light theme:
image

Proposal in dark theme:
image

Also, I noticed two other issues:

  • The weighted edges are not colored differently anymore
  • The styling of the workflow standalone example: i.e. the resize handles differ from the selection color. Should we align that as well?

image

* Added different color for weighted edges
* Tested for color blindness
* Used theia color for selection and handles in standalone
@planger
Copy link
Member Author

planger commented Jan 14, 2022

Thanks @ndoschek!
I used your suggested colors, added different color for weighted edges, tested for color blindness (see below), used theia color for selection and handles in standalone.

Red-Weak/Protanomaly:
image

I think it still is nicely distinguishable.

Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

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

Thanks a lot Philip, looks great to me! 🎉

Could you please update the file headers with the current year here as well, thanks!

@planger
Copy link
Member Author

planger commented Jan 14, 2022

Sure, added 4495c41. Thanks!

@planger planger merged commit 5dad48d into master Jan 17, 2022
@planger planger deleted the planger/issues/492 branch January 17, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants