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

Add dashboard button #6474

Merged
merged 23 commits into from May 23, 2023
Merged

Add dashboard button #6474

merged 23 commits into from May 23, 2023

Conversation

Procrat
Copy link
Contributor

@Procrat Procrat commented Apr 28, 2023

Pull Request Description

Closes #6399: Adding a button to the top bar in the project view to return to the dashboard.

Note that this just fires a DOM event (see #6399). To test it, you can add an event listener: document.addEventListener('show-dashboard', console.log)

dashboard-button2.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the Scala, Java, and Rust style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Procrat Procrat force-pushed the wip/procrat/home-button-6399 branch from bf3f5ba to 660c16d Compare April 28, 2023 16:17
@Procrat Procrat marked this pull request as ready for review April 28, 2023 16:17
app/gui/src/presenter/project.rs Outdated Show resolved Hide resolved
app/gui/view/src/project.rs Outdated Show resolved Hide resolved
app/gui/view/src/project.rs Outdated Show resolved Hide resolved
app/gui/view/src/project.rs Outdated Show resolved Hide resolved
* develop:
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
@somebody1234 somebody1234 mentioned this pull request May 2, 2023
5 tasks
Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

QA passed. I only think we need to change the color of the toggled button because it is not a ToggleButton; it is a regular one. It should not preserve the color between presses.

lib/rust/ensogl/app/theme/hardcoded/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Ilya Bogdanov <fumlead@gmail.com>
@Procrat Procrat added the CI: Ready to merge This PR is eligible for automatic merge label May 3, 2023
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

This code has many places where variables are strangely named, some places where the code is strangely layout. @Procrat did you do self-review before requesting the PR review?

@vitvakatu did you do Q/A only or also code review? In the latter case, the above question applies also to you.

app/gui/src/presenter/project.rs Outdated Show resolved Hide resolved
@@ -416,6 +426,8 @@ impl Project {
view.set_read_only <+ view.toggle_read_only.map(f_!(model.toggle_read_only()));
eval graph_view.execution_environment((env) model.execution_environment_changed(*env));
eval_ graph_view.execution_environment_play_button_pressed( model.trigger_clean_live_execution());

eval_ view.dashboard_button_pressed (model.close_ide());
Copy link
Member

Choose a reason for hiding this comment

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

I find the name dashboard_button_pressed confusing, as there are multiple buttons in the dashboard. Which dashboard button does this code refers to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in the project view, so the only thing it could refer to is the button to go to the dashboard. If there's a name that makes more sense to you, feel free to suggest it.

app/gui/view/graph-editor/src/execution_environment.rs Outdated Show resolved Hide resolved
Comment on lines 453 to 454
space_for_window_buttons (Vector2<f32>),

top_bar_offset_x (f32),
Copy link
Member

Choose a reason for hiding this comment

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

The old code was referring to window buttons that we display sometimes - when the window border is hidden. I see it is removed, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

top_bar_offset_x roughly maintains the same function as space_for_window_buttons, but I gave it a more accurate name, since (1) it's no longer just an offset for window buttons but also the dashboard button, (2) only the offset in the x-axis was used and (3) it's not within the responsibilities of the graph editor to know what sort of items are placed in the top bar by the project view.

Copy link
Member

Choose a reason for hiding this comment

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

As I've written in my previous PR review - top bar is something different than window traffic light buttons. Top bar is of the width of the whole application, so the old names were better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old name is just not true anymore since it's not just space for window buttons. If you can think of a better name, feel free to share it.

app/gui/view/src/project.rs Outdated Show resolved Hide resolved
app/gui/view/src/project/dashboard_button.rs Outdated Show resolved Hide resolved
app/gui/view/src/project/dashboard_button.rs Outdated Show resolved Hide resolved
app/gui/view/src/project/dashboard_button.rs Outdated Show resolved Hide resolved
app/gui/view/src/project/dashboard_button.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/app/theme/hardcoded/src/lib.rs Outdated Show resolved Hide resolved
@vitvakatu
Copy link
Contributor

@vitvakatu did you do Q/A only or also code review? In the latter case, the above question applies also to you.

@wdanilo I don't usually do code reviews, as I don't have write access, and my reviews would not approve the PR anyway. So I was not doing the review in this case, only QA testing.

@wdanilo
Copy link
Member

wdanilo commented May 3, 2023

@vitvakatu did you do Q/A only or also code review? In the latter case, the above question applies also to you.

@wdanilo I don't usually do code reviews, as I don't have write access, and my reviews would not approve the PR anyway. So I was not doing the review in this case, only QA testing.

Ok, makes sense. Regarding code review, feel free to do it. I want to transition to a state when I will not do code review of every PR. In case your review will be on-point, I'd be very happy to just skip reviews on PRs reviewed by you in the future. Regarding approving PRs - we can change it, so your review will approve it, I don't think its connected to write access though.

@vitvakatu
Copy link
Contributor

Not sure how it is called, github is writing about write access:
image

@wdanilo
Copy link
Member

wdanilo commented May 3, 2023

Not sure how it is called, github is writing about write access:
image

Interesting. Anyway, I'd try to reconfigure that in the future. For now, feel free to do code reviews and, as I've written above, if they will be on-point, I'd be more than happy to skip my reviews on PRs reviewed by you then :)

* develop:
  Fix cut-off in text visualisations (#6421)
  Infer correct synthetic name for nested modules (#6525)
  Delete unused websocket dependency (#6535)
  Run typecheck and eslint on `./run lint` (#6314)
  Force pending saves if client closes abruptly (#6514)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
@Procrat
Copy link
Contributor Author

Procrat commented May 4, 2023

This code has many places where variables are strangely named, some places where the code is strangely layout. @Procrat did you do self-review before requesting the PR review?

I find it hard not to interpret that question as a rhetoric one. In case you actually mean it, then yes, you can assume that professional software engineers always self-review before making a PR. Every variable and function name was carefully thought about. In case you encounter unexpected white-space, it's because it was like that before I changed the code and it was deemed as having little value in changing it as part of this PR.

It might help you to realise that if things don't adhere to the specific standards you have in mind, it isn't necessarily out of negligence. Perhaps it's because those standards are peculiar, surprising or unknowable to others.

@Procrat Procrat requested a review from wdanilo May 4, 2023 12:59
@wdanilo
Copy link
Member

wdanilo commented May 4, 2023

This code has many places where variables are strangely named, some places where the code is strangely layout. @Procrat did you do self-review before requesting the PR review?

I find it hard not to interpret that question as a rhetoric one. In case you actually mean it, then yes, you can assume that professional software engineers always self-review before making a PR. Every variable and function name was carefully thought about. In case you encounter unexpected white-space, it's because it was like that before I changed the code and it was deemed as having little value in changing it as part of this PR.

It might help you to realise that if things don't adhere to the specific standards you have in mind, it isn't necessarily out of negligence. Perhaps it's because those standards are peculiar, surprising or unknowable to others.

Oh, that was a serious question, not a rhetoric one. During review I just found several places which were very easy to see when doing fast self-review, like code that was misformatted (with a lot of random spaces in the indentation). I've seen that after fast scrolling the code immediately after opening it and that's why I thought that self-code review was not made here. If it was, and somehow you did not notice it, it's fine. I just preferred to check if the self-code-review is being made :)

The strange whitespacing I'm referring to is for example this code:

image

I don't know if it is an old code moved from other place or a new one, but during self-code-review I'm always catching such things and fixing them, as they make the code really hard to read later to other people. Anyway, in such a case, I'd be just thankful if during self code review you'd also take care of old code that was moved and poorly formatted, ok ? :)

Comment on lines 45 to 49
eval size_update ([model] ((_, size, top_bar_offset_x)) {
let y_offset = MACOS_TRAFFIC_LIGHTS_VERTICAL_CENTER;
let traffic_light_width = traffic_lights_gap_width();

let execution_environment_selector_x = gap_size.x + traffic_light_width;
let execution_environment_selector_x = top_bar_offset_x + traffic_light_width;
Copy link
Member

Choose a reason for hiding this comment

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

As I've written in my previous PR review - top bar is the application "top bar" - a bar that spawns from left to right of the application (occupies whole width of the application). In such a case, I do not understand what is "top_bar_offset_x" and why execution environment is on the right of top_bar. Please refactor the names of the variables in this PR to make them clear for the readers. Execution environment selector is placed inside of the top-bar, not on its right side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was clear to both me and the person who reviewed and approved this code initially. If it's not clear to you, then, please, suggest a name that is clear to you. I can't read your thoughts.

@@ -41,6 +44,9 @@ use ide_view_graph_editor::NodeSource;
/// Browser when user is quickly typing in the expression input.
const INPUT_CHANGE_DELAY_MS: i32 = 200;

/// The height for the top bar.
const TOP_BAR_HEIGHT: f32 = 38.0;
Copy link
Member

Choose a reason for hiding this comment

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

Where does this value come from? Where in Figma do we have top bar with 38 px height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was derived from the current state. Since I thought the top bar was carefully designed, I didn't want to mess it up. Do you want me to change it? I think it's 50px in the Figma design.

Comment on lines 453 to 454
space_for_window_buttons (Vector2<f32>),

top_bar_offset_x (f32),
Copy link
Member

Choose a reason for hiding this comment

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

As I've written in my previous PR review - top bar is something different than window traffic light buttons. Top bar is of the width of the whole application, so the old names were better.

Comment on lines 278 to 286
let top_left = Vector2(-scene_shape.width, scene_shape.height) / 2.0;
if let Some(window_control_buttons) = &*self.window_control_buttons {
let pos = Vector2(-shape.width, shape.height) / 2.0;
window_control_buttons.set_xy(pos);
window_control_buttons.set_xy(top_left);
}
let dashboard_button_offset = Vector2(window_control_buttons_width, 0.0);
let dashboard_button_origin = Vector2(dashboard_button::SIZE, -TOP_BAR_HEIGHT) / 2.0;
let dashboard_button_pos = top_left + dashboard_button_offset + dashboard_button_origin;
self.dashboard_button.set_xy(dashboard_button_pos);
let top_bar_width = dashboard_button_offset.x + dashboard_button::SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of manually lay outing these things, you can use display object's auto layout. Please use it in all modern code. Manual lay outing of things requires al ot of boilerplate and is hard to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using auto layout would require massive refactoring of the whole top bar, we deliberately decided not to do that (as the top bar is not following the exact Figma design at the moment). This kind of refactoring should go into a separate task, this is not a part of adding dashboard button.

Comment on lines 408 to 423
let window_control_buttons_width = if let Some(window_control_buttons) =
&*model.window_control_buttons
{
frp::extend! { network
graph.space_for_window_buttons <+ window_control_buttons.size;
eval_ window_control_buttons.close (model.on_close_clicked());
window_control_buttons_size <- all(init, window_control_buttons.size)._1();
window_control_buttons_width <- window_control_buttons_size.map(|size| size.x);
eval_ window_control_buttons.close (model.on_close_clicked());
eval_ window_control_buttons.fullscreen (model.on_fullscreen_clicked());
}
window_control_buttons_width
} else {
frp::extend! { network
window_control_buttons_width <- init.constant(crate::graph_editor::TOP_BAR_ITEM_MARGIN);
}
window_control_buttons_width
};
Copy link
Member

Choose a reason for hiding this comment

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

No, I'm not talking about "any button definition". I'm talking about this specific button definition. If button is conditionally created, then the code responsible for its creation can attach its events to another FRP network events by using the <+ functionality.

Such a code has also other benefits – allows for showing / hiding window control buttons instead of hardcoding this state forever at the app start.

Comment on lines 27 to 42
/// Defines an icon for returning to the dashboard. It looks like a hamburger button.
mod icon {
use super::*;

use ensogl::data::color;
use ensogl_component::toggle_button::ColorableShape;

ensogl::shape! {
alignment = center;
(style: Style, color_rgba: Vector4<f32>) {
let fill_color = Var::<color::Rgba>::from(color_rgba);
let width = Var::<Pixels>::from("input_size.x");
let height = Var::<Pixels>::from("input_size.y");
let unit = &width / 16.0;
let mid_bar = Rect((&unit * 12.0, &unit * 3.0)).corners_radius(&unit);
let top_bar = mid_bar.translate_y(&unit * -5.0);
Copy link
Member

Choose a reason for hiding this comment

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

We have a lot of icons defined somewhere in a file. Why this icon is not defined among other icons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is where it's logically used? Why would you put it alongside unrelated icons?

app/gui/view/graph-editor/src/component/profiling.rs Outdated Show resolved Hide resolved
* develop: (28 commits)
  Add tests for Date.until, Date.next and Date.previous. (#6606)
  Improve `Non_Unique_Primary_Key` error, split file format detection into read/write, improve SQLite format detection (#6604)
  tokenize_to_columns or parse_to_columns results in a single column we shouldn't add the  1 (#6607)
  Fix node editing race condition (#6594)
  Add format to the in-memory Column (#6538)
  Fix dashboard issues (part 2) (#6511)
  Fix visualisation type selector artifacts rendered after node preview visualisation was closed. (#6575)
  Revert typescript CI Lint changes (#6602)
  Fix the Engine version check in GUI (#6570)
  Show error pop-up when failing to rename a project (#6366)
  Small changes from Book Club issues (#6533)
  "at_least_one" flag for tokenize_to_rows (#6539)
  Benchmark Engine job runs only engine, not Enso benchmarks (#6534)
  Catch 5813 and avoid crash (#6585)
  Fix opening links in desktop IDE (#6507)
  Identify SyntaxError exception and avoid printing a stack trace (#6574)
  Fix dashboard issues (#6502)
  Let ChangesetBuilder.invalidated search even container elements (#6548)
  Fix #5075: stop panning on full-screen visualisation (#6530)
  Only `Join_Kind.Inner` removes the common-named columns (#6564)
  ...
@Procrat Procrat requested a review from wdanilo May 10, 2023 08:37
* develop:
  Limit the number of reported warnings (#6577)
  Add COOP+COEP+CORP headers (#6597)
  Fix issues with missing sourcemaps (#6572)
  Fix asset delete; implement project delete and project rename (#6566)
  Fix #6377: Change ctrl-r shortcut (#6620)
* develop:
  Implement loading spinner for visualisations. (#6512)
  Fix blank input port (#6614)
  Add `Date_Range` (#6621)
  All Vector operations shall be applicable on java.util.ArrayList (#6642)
  Fix redirect paths and enable authentication and new dashboard by default (#6605)
  Fix #6287: wrong nested breadcrumb ordering (#6617)
  Whitelist AWS Cognito domains (#6643)
  Revert "Add COOP+COEP+CORP headers (#6597)" (#6647)
  Fix shortcuts table formatting (#6644)
  Automatic type based dropdown does not include singleton in a union type (#6629)
  Make Meta.get_annotation work for constructor (#6633)
@Procrat Procrat requested review from wdanilo and removed request for wdanilo May 12, 2023 09:24
@Procrat
Copy link
Contributor Author

Procrat commented May 12, 2023

@wdanilo: I've made the changes you mentioned in our call. Could you have another look?

-MACOS_TRAFFIC_LIGHTS_SIDE_OFFSET - MACOS_TRAFFIC_LIGHTS_CONTENT_HEIGHT / 2.0;
const MAX_ZOOM: f32 = 1.0;
/// Space between items in the top bar.
const TOP_BAR_ITEM_MARGIN: f32 = 10.0;
pub const TOP_BAR_ITEM_MARGIN: f32 = 10.0;
Copy link
Member

Choose a reason for hiding this comment

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

This is defined as 16 in Figma.

Comment on lines 273 to 289
&self,
scene_shape: &display::scene::Shape,
window_control_buttons_width: f32,
) -> f32 {
let top_left = Vector2(-scene_shape.width, scene_shape.height) / 2.0;
self.window_control_buttons.set_xy(top_left);
let go_to_dashboard_button_offset = Vector2(window_control_buttons_width, 0.0);
let go_to_dashboard_button_origin = Vector2(
go_to_dashboard_button::SIZE / 2.0,
crate::graph_editor::MACOS_TRAFFIC_LIGHTS_VERTICAL_CENTER,
);
let go_to_dashboard_button_pos =
top_left + go_to_dashboard_button_offset + go_to_dashboard_button_origin;
self.go_to_dashboard_button.set_xy(go_to_dashboard_button_pos);
let project_view_top_bar_width =
go_to_dashboard_button_offset.x + go_to_dashboard_button::SIZE;
project_view_top_bar_width
Copy link
Member

Choose a reason for hiding this comment

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

I was writing about in in my previous PR review - this can use auto-layout for window control buttons and the "go to dashboard" button. Is there a reason auto-layout is not used here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a reason: it wasn't there before, and was implemented this way. Changing to automatic layouts could be smooth, but also it could not.

I'd wait with changing to autolayout to a moment when we make a new, single top bar component.

Comment on lines 1 to 21
use ensogl::prelude::*;

use ensogl::application::Application;
use ensogl::display;



#[derive(Clone, CloneRef, Debug, Deref)]
pub struct TopBarComponent<T: CloneRef> {
root: display::object::Instance,
#[deref]
view: T,
}

impl<T: CloneRef + display::Object> TopBarComponent<T> {
pub fn new(app: &Application, view: T) -> Self {
let scene = &app.display.default_scene;
let root = display::object::Instance::new();
scene.layers.panel.add(&root);
Self { root, view }
}
Copy link
Member

Choose a reason for hiding this comment

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

no documentation

* develop:
  Build nightlies every day. (#6681)
  Force `newDashboard` default on the CI-built packages. (#6680)
  Verify ascribed types of parameters really exist (#6584)
  SuggestionBuilder needs to send ascribedType of constructor parameters (#6655)
  Improvements to the Table visualization. (#6653)
  Removing flush (#6670)
  Improving widgets for take/drop (#6641)
  disable authentication and newDashboard by default (#6664)
  Add COOP+COEP+CORP headers (#6646)
@Procrat Procrat force-pushed the wip/procrat/home-button-6399 branch from 44e16ce to 0a6d50e Compare May 16, 2023 12:19
@Procrat Procrat requested a review from wdanilo May 16, 2023 12:20
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Minor comments from my side. I think that most of them I added here in the past, but they might have been closed. Anyway, if either of this comments is not applicable, let's have a short fast call please :)

Comment on lines 21 to 22
/// The gap in pixels between the various components of the project view top bar.
const GAP: f32 = 18.0;
Copy link
Member

Choose a reason for hiding this comment

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

I've written about it in the previous PR review - please take the values from the Figma design. The gap between components in the top bar is 16, not 18 pixels.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand the confusion. I took from here:
x

But don't worry, I'll add the 2 pixels padding to the icon and change the gap to 16 pixels.

Copy link
Member

Choose a reason for hiding this comment

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

That's measurement is not strictly correct. The right icon's width is 16 pixels, but its content is 12 pixels wide and it has 2 pixels of free space on both left and right side INSIDE of the icon. So the spacing between the icons is 16 px, and what you observe is just a part of the icon being transparent.

Comment on lines 23 to 24
/// The padding around the project view top bar.
const PADDING: f32 = 18.0;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this - what is this padding? What does it mean that this is "around" the project view top bar? Also, what is "project view top bar" and which gap in Figma this 18 pixels space refers to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to be a bit more accurate now. Have another look. PADDING_LEFT is now the padding to the left of the top bar (and changed to 19px as in the Figma design). "Project view top bar" is the name you decided on during our long call two weeks ago.

Comment on lines 28 to 46
mod icon {
use super::*;

use ensogl::data::color;
use ensogl_component::toggle_button::ColorableShape;

ensogl::shape! {
(style: Style, color_rgba: Vector4<f32>) {
let fill_color = Var::<color::Rgba>::from(color_rgba);
let width = Var::<Pixels>::from("input_size.x");
let height = Var::<Pixels>::from("input_size.y");
let unit = &width / SIZE.x;
let mid_bar = Rect((&unit * 12.0, &unit * 3.0)).corners_radius(&unit);
let top_bar = mid_bar.translate_y(&unit * -5.0);
let bottom_bar = mid_bar.translate_y(&unit * 5.0);
let all_bars = top_bar + mid_bar + bottom_bar;
let shape = all_bars.fill(fill_color);
let hover_area = Rect((&width, &height)).fill(INVISIBLE_HOVER_COLOR);
(shape + hover_area).into()
Copy link
Member

Choose a reason for hiding this comment

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

I think I was writing about it in one of the previous PR reviews, but I don't see my comment opened (maybe it was closed and it's hard to find it now). We have a module which defines a lot of icons - why we are defining this icon here rather than defining it where other app icons are defined?

Copy link
Contributor Author

@Procrat Procrat May 22, 2023

Choose a reason for hiding this comment

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

The comment is still here. Yes, there are a lot of comments by now unfortunately.

If I'm not mistaken, icons across the codebase are generally defined in the place where they are used, just like other code, which makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

  1. I don't fully understand your sentence icons across the codebase are generally defined in the place where they are just, just like other code, which makes sense to me. - are you sure it contains all words / things you wanted to include in it?
  2. The idea is to put icons in one place, so they can be reused by different parts of the application. This icon can be used in more places than only for the "go to dashboard" button. It can be used as a hamburger menu icon, for example. So, we have somewhere in the codebase a place with a lot of icons registered by name and if we can, let's move this icon there and just use it from our "icons database", ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wojciech means this place: https://github.com/enso-org/enso/blob/46418f132d365ae1946710a445ce3a7cf9d52343/app/gui/view/component-browser/component-list-panel/grid/src/entry/icon.rs

However, this place does not contain all icons. For example, icons for nodes are not here. And it has some limitations, most prominently: the icon size is constant, set to 16. I see the dashboard button has a size of 18px, so it does not fit here. Also, the location of the file must change if we want to make it having all icons (currently it's code specific to the Component Browser).

I think we should generalize the code (and put all icons there, e.g. node's icon) before requiring adding icons there from PRs.

Copy link
Member

Choose a reason for hiding this comment

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

@farmaazon thanks for the link!

The go to dashboard icon size is 16x16, not 18x18, so it fits there. All of our icons in Figma are 16x16 to keep them consistent :)

Regarding generalizing that place – I'm normally doing code generalization when new things appear that require generalization. Back then, that was the only place with icons. Now, we are using icons in other places as well, so it's natural to me to generalize it now (if I was the author of this PR, I'd do this generalization). Otherwise, we are just making it harder to generalize in the future.

@@ -0,0 +1,199 @@
//! The component with buttons in the top left corner. See [[View]].
Copy link
Member

Choose a reason for hiding this comment

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

The file is named "window_control_buttons", while the module docs tell that this is a component with buttons in the top left corner. So is this module defining window control buttons or just a component with buttons in some corner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just moved code. Let's not re-evaluate all code just when it gets moved.

Copy link
Member

Choose a reason for hiding this comment

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

We are re-evaluating moved code in other PRs. If the docs does not make sense, they need to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are re-evaluating moved code in other PRs

Do we? If yes, then I'm against it as a rule, as this make a delivery of tasks very unpredictable. This very PR was meant to implement a 1-day task with high priority, we definitely should not reevaluate older code.

Copy link
Member

Choose a reason for hiding this comment

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

I have pretty often comments in my PR from different people, uncluding you Adam, about old code that was moved that is not up to date anymore. Some things, like this thing, where the docs are just unclear and fixing them is 1-minute job should be fixed if the reviewer catches them - this is also what we've been doing in many PRs in the past.

Nevertheless, @Procrat, please fix these docs in the next PR, as this PR was already merged.

pub struct Model {
app: Application,
display_object: display::object::Instance,
shape: shape::View,
Copy link
Member

Choose a reason for hiding this comment

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

Why this shape is needed? It is behind buttons, so looks like it is needed for some mouse interaction, but it is not described here. Also, its name does not explain its purpose. The name "shape" does not semantically tell what it is used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old code.

Comment on lines +28 to +37
mod shape {
use super::*;
shape! {
alignment = center;
(style: Style) {
Plane().fill(INVISIBLE_HOVER_COLOR).into()
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Is this new or old code? In either case, we should use the box shape here to lower the amount of draw calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is old code.

Comment on lines +176 to +181
impl Deref for View {
type Target = Frp;
fn deref(&self) -> &Self::Target {
&self.frp
}
}
Copy link
Member

Choose a reason for hiding this comment

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

such things can be derived

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is old code that's just been moved.

Copy link
Member

Choose a reason for hiding this comment

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

Great, can you derive it? Probably deriving it would take less time than this conversation here :)

@farmaazon
Copy link
Contributor

farmaazon commented May 18, 2023

The issue is blocking @sylwiabr and cloud from testing new features. I'd opt for merging it now and fix review comments later (especially they in majority are about file which seems to be older code).

@wdanilo could you unblock this PR?

@Procrat Procrat requested a review from wdanilo May 22, 2023 08:39
* develop: (30 commits)
  Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768)
  Implement simple variants of `parse` for the Database backend (#6731)
  Enable `require-jsdoc` lint and add two lints related to React (#6403)
  Decimal/Integer .round and .int #6654 (#6743)
  Set suggestion reexports when serializing the library (#6778)
  Fix file uploading node expression. (#6689)
  Using WarningsLibrary to query for warnings (#6751)
  Implement `cast` for Table and Column (#6711)
  Display Initializing project... message when initializing project (#6661)
  Only send suggestions updates when type changes (#6755)
  sbt runEngineDistribution --debug to ease debuggging (#6745)
  Display "modified at" column on the cloud dashboard (#6687)
  Meta.meta Integer . methods (#6740)
  Show spinner while loading directory (#6714)
  Add cloud dashboard to changelog (#6688)
  Fix list editor panics during insertion (#6540)
  Update bug-report.yml
  Remove project create form (#6710)
  Change full-screen visualisation shortcut to shift-space (#6663)
  Revert "Show spinner when opening/creating a project (#6321)" (#6712)
  ...
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

@farmaazon sure thing, here is the approval. However, I'd still like to resolve some of the ongoing conversations, so let's continue them / fix them in this or the next PR then, please.

* develop:
  Link to new 101 tutorial and not deprecated one. (#6793)
  Add logs section to the bug template (#6798)
  Fix #6521: Main module function calls shouldn't use project namespace (#6719)
  Add project creation time to project metadata (#6780)
  Add a compiler pass to analyze non-existing imported symbols (#6726)
@farmaazon
Copy link
Contributor

QA Report: 🟢

But please update the PR description for future generations: the event is 'show-dashboard', not 'ide-close'.

* develop:
  Use state sent with `GET /directories` instead of querying state separately (#6794)
  Run TypeScript typechecking and eslint on Lint CI (#6603)
  Disable cloud backend for unverified users; use local backend as default backend; minor bugfixes (#6779)
  Reloading file in LS after desynchronization. (#6752)
  Missing conversion of hash key in EqualsNode (#6803)
  feat: set constructor args tag values (#6801)
@mergify mergify bot merged commit 8e62ed6 into develop May 23, 2023
24 checks passed
@mergify mergify bot deleted the wip/procrat/home-button-6399 branch May 23, 2023 14:23
@somebody1234
Copy link
Collaborator

is the dashboard button supposed to show up over the fullscreen viz?
image

@wdanilo
Copy link
Member

wdanilo commented May 30, 2023

is the dashboard button supposed to show up over the fullscreen viz?
image

Of course not :)

@somebody1234
Copy link
Collaborator

somebody1234 commented May 30, 2023

for posterity: an issue has been opened at #6881
but it's a duplicate of #6839, itself a duplicate (kinda) of #6722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enso workspace - go to project dashboard button
5 participants