Close and Fullscreen buttons for IDE in the cloud #1511
Conversation
mwu-tow
added
Difficulty: Core Contributor
…on-1095 # Conflicts: # src/rust/ide/view/graph-editor/src/lib.rs
There was a problem hiding this comment.
I left few comments, but overall, the quality of the code is very high. I'm surprised by the amount of the code, but there is a lot of refactoring / generalization here, which I like a lot. After reading the whole PR, I'm really happy its done. There is only one place that should be merged with other part of the codebase (and which probably owuld have saved you a lot of work - we already have buttons generalization).
Also, please provide here screenshots + screencast of how the buttons behave on hover, clicked, and when the window is being resized! :)
| @@ -377,6 +377,11 @@ define_color_spaces! { | |||
| // =========== | |||
|
|
|||
| impl Rgb { | |||
| /// Construct RGB color by mapping [0 – 255] value range into [0.0 – 1.0]. | |||
| pub fn from_integral_range(r:impl Into<f32>, g:impl Into<f32>, b:impl Into<f32>) -> Self { | |||
There was a problem hiding this comment.
I dont understand the name. Wjy there is "rannge" in name? We are consturctng colors not from a range, but from a 255-integer color encoding. I just think that the "range" word is very confusing here
src/rust/ensogl/lib/core/src/display/shape/primitive/def/var.rs
Outdated
Show resolved
Hide resolved
| impl Var<color::Rgba> { | ||
| /// Build a color from its components. | ||
| pub fn srgba(r:impl Into<Var<f32>>, g:impl Into<Var<f32>>, b:impl Into<Var<f32>>, a:impl Into<Var<f32>>) | ||
| -> Var<color::Rgba> { | ||
| format!("srgba({},{},{},{})", | ||
| r.into().glsl(), | ||
| g.into().glsl(), | ||
| b.into().glsl(), | ||
| a.into().glsl() | ||
| ).into() | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be implemented as .glsl() method on colors instead imo. This is how we are converting all values there. Moreover, I see inconsistency here - srgba is a name used in GLSL due to convention there. However, in Rust, the counterpart of srgba is color::Rgba, while the counterpart of GLSL's rgba is color::LinearRgba. So the name srgba in Rust can be confusing.
There was a problem hiding this comment.
This cannot be a glsl method, as the purpose is to construct Var<color:Rgba>, not Glsl.
Also, it cannot be on color, as there is no color, just the variables with its components.
| #[derive(Clone,Copy,Debug)] | ||
| pub enum State { | ||
| /// Base look when button is neither hovered nor pressed. | ||
| /// Also used when button was pressed but mouse was hovered out. | ||
| Unconcerned, | ||
| /// Look when button is hovered but not pressed. | ||
| Hovered, | ||
| /// Look when button is being pressed (held down) with mouse hovered. | ||
| Pressed, | ||
| } |
There was a problem hiding this comment.
We already have an abstraction for buttons! @MichaelMauderer was writing one! Please merge them together if possible.
| |strict_hover,nearby_hover,clicked| { | ||
| match (strict_hover,nearby_hover,clicked) { | ||
| (true , _ , true) => State::Pressed, | ||
| (_ , true , _ ) => State::Hovered, | ||
| (_ , _ , true) => State::Hovered, | ||
| _ => State::Unconcerned, | ||
| } |
There was a problem hiding this comment.
This looks like our already exisitng button generalization code. Yeah, they need to be merged.
| let radius_frp = style.get(ensogl_theme::application::window_control_buttons::radius); | ||
|
|
||
| // Style's relevant color FRP endpoints. | ||
| let background_unconcerned_color = style.get_color(Shape::background_color_path(State::Unconcerned)); |
| let default_icon_color_path = Shape::icon_color_path(State::Unconcerned); | ||
| let default_icon_color = style.get_color(default_icon_color_path).value(); | ||
| let icon_color = color::Animation::new(&network); | ||
| icon_color.target(color::Lcha::from(default_icon_color)); |
There was a problem hiding this comment.
you may also consider call "skip" on animation.
Pull Request Description
This PR:
is_in_cloud. Currently it is set to false, it is expected that when IDE is running in the clout, it will be set to true.Along with these there are a number of improvements and utilities that I found needed at various points of developing this feature.
Important Notes
Checklist
Please include the following checklist in your PR:
CHANGELOG.mdwas updated with the changes introduced in this PR.- [ ] All code has automatic tests where possible.- [ ] All code has been profiled where possible.