Sync breadcrumbs and documentation panel. #7508

MichaelMauderer · 2023-08-07T12:58:25Z

Pull Request Description

Implements #7310.

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.

vitvakatu · 2023-08-07T13:36:46Z

lib/rust/ensogl/component/breadcrumbs/src/entry.rs

@@ -186,8 +186,9 @@ impl EntryData {
        let separator = separator::View::new();
        let state = default();
        let icon: any_icon::View = default();
-        icon.set_size((ICON_WIDTH, ICON_WIDTH));
+        ellipsis.set_size((ICON_WIDTH, ICON_WIDTH));


The next line sets the size of the ellipsis as well.

vitvakatu · 2023-08-07T13:37:27Z

lib/rust/ensogl/component/breadcrumbs/src/entry.rs

+        self.icon.set_x(-size.x / 2.0 - 2.0);
+        self.icon.set_y(-ICON_WIDTH * 1.25 - 2.0);


Offsets like 2.0 and 1.25 should probably be moved to constants.

vitvakatu · 2023-08-07T13:39:45Z

app/gui/view/component-browser/component-list-panel/Cargo.toml

@@ -18,6 +18,7 @@ ensogl-selector = { path = "../../../../../lib/rust/ensogl/component/selector" }
 ensogl-toggle-button = { path = "../../../../../lib/rust/ensogl/component/toggle-button" }
 ensogl-text = { path = "../../../../../lib/rust/ensogl/component/text" }
 ensogl-tooltip = { path = "../../../../../lib/rust/ensogl/component/tooltip/" }
+ensogl-breadcrumbs = { path = "../../../../../lib/rust/ensogl/component/breadcrumbs/" }


Is it used in this crate?

No. Removed.

vitvakatu · 2023-08-07T13:44:28Z

app/gui/src/controller/searcher.rs

+    /// Return the documentation for the breadcrumb.
+    pub fn documentation_for_selected_breadcrumb(&self) -> Option<EntryDocumentation> {
+        let selected = self.breadcrumbs.selected();
+        console_log!("documentation_for_selected_breadcrumb -> selected: {:?}", selected);


console_log left in the code.

vitvakatu · 2023-08-07T13:48:31Z

app/gui/src/controller/searcher.rs

+                None
+            }
+        } else {
+            warn!("Update readcrumbs called with invalid index: {}", index);


Suggested change

warn!("Update readcrumbs called with invalid index: {}", index);

warn!("Update breadcrumbs called with invalid index: {}", index);

vitvakatu · 2023-08-07T13:54:35Z

app/gui/src/controller/searcher/breadcrumbs.rs

+    }
+
+    /// Return a list of breadcrumbs to be displayed in the panel.
+    pub fn items(&self) -> impl Iterator<Item = BreadcrumbEntry> + '_ {


It looks suspicious lifetime-wise, and it seems to be unused.

vitvakatu · 2023-08-07T13:56:02Z

app/gui/src/controller/searcher.rs

+        let name = &component.name;
+        let icon = Some(component.icon());
+        let module = &component.defined_in;
+        let breadcrumbs_base = module.path().iter().map(|name| Breadcrumb::new_without_icon(name));


Here I see that we create breadcrumbs without icons for the modules path, but in the video, there are icons on each crumb. Is that correct?

This was left over and unused, but you are right that only the last breadcrumb should have an icon. Fixed.

vitvakatu

Code changes look good to me. @MichaelMauderer could you update the screencast in the PR description?

MichaelMauderer · 2023-08-09T15:07:38Z

Code changes look good to me. @MichaelMauderer could you update the screencast in the PR description?

Updated.

kazcw · 2023-08-10T04:01:23Z

QA: 🟡

When the documentation is hidden (with the Show/hide documentation button), the breadcrumb container remains:

Other than that, it looks good.

farmaazon · 2023-08-10T07:11:33Z

When the documentation is hidden (with the Show/hide documentation button), the breadcrumb container remains:

This issue is also on develop, and I assume it will be fixed in the documentation panel's update. I would merge it.

MichaelMauderer · 2023-08-10T08:49:00Z

When the documentation is hidden (with the Show/hide documentation button), the breadcrumb container remains:

This issue is also on develop, and I assume it will be fixed in the documentation panel's update. I would merge it.

Yes, this is addressed in that PR. So I'll merge this one.

…r/synchronize-cb-breadcrumbs-with-documentation-panel # Conflicts: # app/gui/src/controller/searcher.rs

Sync breadcrumbs and documentation panel.

c379783

MichaelMauderer added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 7, 2023

MichaelMauderer self-assigned this Aug 7, 2023

MichaelMauderer marked this pull request as ready for review August 7, 2023 12:59

MichaelMauderer requested review from farmaazon, kazcw, vitvakatu, Frizi and mwu-tow as code owners August 7, 2023 12:59

vitvakatu reviewed Aug 7, 2023

View reviewed changes

Review feedback.

68fbbb5

MichaelMauderer requested a review from vitvakatu August 9, 2023 13:25

vitvakatu approved these changes Aug 9, 2023

View reviewed changes

MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Aug 10, 2023

MichaelMauderer added 2 commits August 15, 2023 10:40

Fix tests.

a151c89

Merge remote-tracking branch 'origin/develop' into wip/MichaelMaudere…

4fc660a

…r/synchronize-cb-breadcrumbs-with-documentation-panel # Conflicts: # app/gui/src/controller/searcher.rs

mergify bot merged commit 7f19b09 into develop Aug 15, 2023
24 checks passed

mergify bot deleted the wip/MichaelMauderer/synchronize-cb-breadcrumbs-with-documentation-panel branch August 15, 2023 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync breadcrumbs and documentation panel. #7508

Sync breadcrumbs and documentation panel. #7508

MichaelMauderer commented Aug 7, 2023 •

edited

Loading

vitvakatu Aug 7, 2023

vitvakatu Aug 7, 2023

MichaelMauderer Aug 9, 2023

vitvakatu Aug 7, 2023

MichaelMauderer Aug 9, 2023

vitvakatu Aug 7, 2023

vitvakatu Aug 7, 2023

vitvakatu Aug 7, 2023

MichaelMauderer Aug 9, 2023

vitvakatu Aug 7, 2023

MichaelMauderer Aug 9, 2023

vitvakatu left a comment

MichaelMauderer commented Aug 9, 2023

kazcw commented Aug 10, 2023

farmaazon commented Aug 10, 2023 •

edited

Loading

MichaelMauderer commented Aug 10, 2023

		self.icon.set_x(-size.x / 2.0 - 2.0);
		self.icon.set_y(-ICON_WIDTH * 1.25 - 2.0);

	warn!("Update readcrumbs called with invalid index: {}", index);
	warn!("Update breadcrumbs called with invalid index: {}", index);

Sync breadcrumbs and documentation panel. #7508

Sync breadcrumbs and documentation panel. #7508

Conversation

MichaelMauderer commented Aug 7, 2023 • edited Loading

Pull Request Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu left a comment

Choose a reason for hiding this comment

MichaelMauderer commented Aug 9, 2023

kazcw commented Aug 10, 2023

QA: 🟡

farmaazon commented Aug 10, 2023 • edited Loading

MichaelMauderer commented Aug 10, 2023

MichaelMauderer commented Aug 7, 2023 •

edited

Loading

farmaazon commented Aug 10, 2023 •

edited

Loading