Documentation panel redesign #7372

vitvakatu · 2023-07-23T14:01:42Z

Pull Request Description

Based on #7350

Closes #7356

Documentation parser now supports two new DocSection types:
- List is a better representation of unordered lists anywhere in the text.
- Arguments, a list starting with the Arguments: keyword. Each item in the list is split into argument names and descriptions, and we use this information to highlight argument names in the documentation.
  I'm not confident with my changes to the doc parser, especially due to the fact there were exactly zero tests for it.
Small adjustments to the documentation CSS. It now looks much closer to the design, though not ideal. In particular, paddings are different now, and no topmost headers are displayed. (as they are replaced with the breadcrumbs in the new design) I also used fixed sizes like 4px instead of 0.25rem to simplify matching with the design. I believe it is the way as we want the most accurate translation from Figma, even though it is generally not the best practice.
Improvements for tags styles. The proper implementation (with different colors, limited count, custom per-tag formatting, ... button...) will require more effort.
Breadcrumbs are in almost proper position now. The exact positioning should be done after updating them to the new design.
Added a missed support for code tags in the documentation. Now text emphasized with backticks has a separate background color.

2023-07-23.17-34-13.mp4

Important Notes

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.

farmaazon · 2023-07-27T13:51:06Z

app/gui/suggestion-database/src/documentation_ir.rs

 /// No documentation is available for the entry.
-#[derive(Debug, Clone, CloneRef, PartialEq)]
+#[derive(Debug, Clone, Copy, CloneRef, PartialEq)]
 #[allow(missing_docs)]
 pub enum Placeholder {
    /// Documentation is empty.
    NoDocumentation,
-    /// Documentation for the Virtual Component group.
-    VirtualComponentGroup { name: ImString },
 }


I guess we could make it a structure? Or it's not needed anymore?

It's not needed anymore and I removed it.

kazcw · 2023-07-27T13:53:40Z

app/gui/view/documentation/src/lib.rs

-        self.breadcrumbs.set_xy(Vector2(0.0, size.y + 36.0));
-        self.breadcrumbs.frp().set_size(Vector2(size.x, 32.0));
-    }
-
    /// Display the documentation and scroll to the qualified name if needed.


This comment seems outdated (already before this change)

kazcw · 2023-07-27T13:56:01Z

app/gui/view/documentation/src/lib.rs

+        let size = Vector2(style.width * width_factor, style.height);
+        self.overlay.set_size(size);
+        let dom_size = Vector2(size.x, size.y - style.breadcrumbs_height);
+        self.dom.set_dom_size(dom_size);
+        self.dom.set_xy(dom_size / 2.0);
+
+        self.background.set_color(style.background);
+        self.background.set_corner_radius(style.corner_radius);
+        self.background.set_size(size);
+
        self.overlay.set_corner_radius(style.corner_radius);
-        self.outer_dom.set_style_or_warn("border-radius", format!("{}px", style.corner_radius));
-        self.inner_dom.set_style_or_warn("border-radius", format!("{}px", style.corner_radius));
-        let bg_color = style.background.to_javascript_string();
-        self.outer_dom.set_style_or_warn("background-color", bg_color);
+        self.dom.set_style_or_warn("border-radius", format!("{}px", style.corner_radius));
+
+        self.breadcrumbs.set_xy(Vector2(0.0, size.y));
+        self.breadcrumbs.frp().set_size(Vector2(size.x, style.breadcrumbs_height));


I don't understand how these lines are organized--it is partially by the component, but some of the corner_radius configuration has its own section?

I've moved them to be organised by component.

kazcw · 2023-07-27T14:15:13Z

lib/rust/parser/doc-parser/src/doc_sections.rs

@@ -155,6 +186,9 @@ impl<L> TokenConsumer<L> for DocSectionCollector {
    fn enter_keyed_section(&mut self, header: Span<'_, L>) {
        self.finish_section();
        let key = header.to_string();
+        if key.to_lowercase() == "arguments" {


Suggested change

if key.to_lowercase() == "arguments" {

if key.eq_ignore_ascii_case("Arguments") {

We process a lot of documentation; it's important to avoid allocations when we can.

kazcw · 2023-07-27T14:36:27Z

lib/rust/parser/doc-parser/src/doc_sections.rs

+            Some(DocSection::Tag { .. })
+            | Some(DocSection::List { .. })
+            | Some(DocSection::Arguments { .. })
+            | None => self.sections.push(DocSection::Paragraph { body: text }),


This is causing the empty Paragraph between the List and the "Example:" section in the new test. The empty paragraph is not needed; the newlines between sections are implicit.

Suggested change

Some(DocSection::Tag { .. })

| Some(DocSection::List { .. })

| Some(DocSection::Arguments { .. })

| None => self.sections.push(DocSection::Paragraph { body: text }),

Some(DocSection::List { .. }) | Some(DocSection::Arguments { .. }) => (),

Some(DocSection::Tag { .. }) | None => self.sections.push(DocSection::Paragraph { body: text }),

kazcw · 2023-07-27T14:40:51Z

lib/rust/parser/doc-parser/src/doc_sections.rs

-            ScopeType::List => self.current_body.push_str("</ul>"),
-            ScopeType::ListItem => (),
+            ScopeType::List => {
+                let items = mem::take(&mut self.current_list);


This gives away our buffer. If we take it, we need log(size) allocations per list. If we iterate/clone it and then clear it, we only need one allocation per List.

kazcw · 2023-07-27T14:42:20Z

lib/rust/parser/doc-parser/src/doc_sections.rs

+                }
+            }
+            ScopeType::ListItem => {
+                self.current_list.push(mem::take(&mut self.current_body));


Suggested change

self.current_list.push(mem::take(&mut self.current_body));

self.current_list.push(self.current_body.drain(..).collect());

kazcw · 2023-07-27T14:45:47Z

lib/rust/parser/doc-parser/src/lib.rs

+                }].to_vec()
+            },
+            List { items: ["List item 1".into(), "List item 2".into(), "List item 3".into()].to_vec() },
+            Paragraph { body: "".into() },


Spurious, see above.

somebody1234

it's the same (automatic) package-lock.json change as in some other PRs.
when merging with develop, this change should go away, because said change has been merged into develop:

enso/app/ide-desktop/package-lock.json

Line 447 in 73237b7

"enso-chat": "git://github.com/enso-org/enso-bot",

Added padding_x and padding_y to hardcoded theme's breadcrumb settings to ensure consistent padding. Also, these padding settings and breadcrumb_height are now used directly in the Style structure, eliminating hardcoded values in the view documentation.

Frizi · 2023-08-08T12:35:53Z

QA Status: 🔴

Overall it looks good, but there are still some minor rendering issues in some edge cases. It looks like some of them are caused by the documentation not having a consistent syntax in a few places.

Some methods have truncated argument lists. The documentation is not properly formatted, such that the argument list is missing a colon after the argument name. Though I think that it should be displayed in some way anyway. Instead, now is displayed as a list with empty items.

We are currently displaying all tags. I don't think that ICON tag should really be shown. We already display the icon itself after all. Though this is not a big issue and might be addressed later if needed.

The selection behaves strangely right after typing. There are two distinct selection indicators (do we still need that?), and they point to different entries. Additionally, the list is scrolled up, such that the actual selected entry is partially outside the view.

EDIT: I think this was a separate issue that is already fixed on develop (hence I have only noticed it in this branch). Updating the branch should resolve it.

cb-strange-selection.mp4

vitvakatu · 2023-08-08T13:14:21Z

I'll take it from here, thanks for testing.

vitvakatu · 2023-08-09T11:53:22Z

@Frizi I fixed both issues.

Now we correctly parse space-separated arguments.
Icon tag is now ignored in the documentation.

Could you retest?

xvcgreg · 2023-08-11T05:13:14Z

@Frizi can you retest it?

Frizi · 2023-08-11T12:58:10Z

QA Status: 🟡

The above issues has been fixed, but i've noticed that invalid behavior of the documentation show/hide action has been reintroduced. The content of the panel should not be reflowed on every frame of the animation. Instead, it should be clipped to fit the animated background size, without acutally changing the content width of underlying DOM element.

See point 2. in #7350 (comment)

No other issues spotted, so once show/hide is fixed it's good to go.

vitvakatu self-assigned this Jul 23, 2023

vitvakatu requested review from MichaelMauderer, farmaazon, kazcw, Frizi, mwu-tow and JaroslavTulach as code owners July 23, 2023 14:01

farmaazon assigned MichaelMauderer Jul 27, 2023

farmaazon approved these changes Jul 27, 2023

View reviewed changes

kazcw approved these changes Jul 27, 2023

View reviewed changes

Base automatically changed from wip/vitvakatu/cb-panel to develop July 31, 2023 15:33

mergify bot requested review from PabloBuchu, indiv0 and somebody1234 as code owners July 31, 2023 15:33

somebody1234 approved these changes Jul 31, 2023

View reviewed changes

MichaelMauderer linked an issue Aug 1, 2023 that may be closed by this pull request

Improved documentation view, as in the design. #7356

Closed

vitvakatu and others added 14 commits August 1, 2023 13:11

Support arguments list in the doc parser

b183b30

Support new doc sections in documentation panel

6c0270a

Remove headers

0544d8a

Remove outer dom and place breadcrumbs

9cad2b8

Fix methods icon

9ae8334

Use unordered list class in css

b0ba0cb

Improve tags styles

60dae86

Remove virtual component groups docs

aca0fa6

Cleanup top-level css styles

082b0ea

Small adjustments to headers

42c343a

Add styles for emphasized text

a5c36a7

Add bold font for arguments

b405887

Self-review

9315b20

Remove redundant placeholder struct.

34c8b00

MichaelMauderer added 6 commits August 1, 2023 13:12

Update outdated doc.

b43362b

Avoid allocation when comparing strings.

22eb8fb

Avoid empty paragraph.

f5b4393

Reduce allocations.

dd1b704

Update test to remove empty paragraph.

3f8bf91

Fix rebase issues.

6658bb8

MichaelMauderer force-pushed the wip/vitvakatu/doc-panel-redesign branch from 4c2beed to 6658bb8 Compare August 3, 2023 09:58

MichaelMauderer added 2 commits August 3, 2023 11:16

Adjusted breadcrumb background dimensions calculation.

367c028

vitvakatu added 3 commits August 9, 2023 14:32

Add support for improper arguments formatting in documenation comments

ae4c314

Do not include Icon tag into the docs

7070403

Merge branch 'develop' into wip/vitvakatu/doc-panel-redesign

ed3c5ac

vitvakatu added 3 commits August 14, 2023 12:35

Fix documentation panel resizing

6f1a08b

enso-formatter

a1c7c3c

Merge branch 'develop' into wip/vitvakatu/doc-panel-redesign

dbcecaf

vitvakatu added CI: Ready to merge This PR is eligible for automatic merge CI: No changelog needed Do not require a changelog entry for this PR. labels Aug 14, 2023

vitvakatu merged commit 02ba9a1 into develop Aug 15, 2023
27 of 29 checks passed

vitvakatu deleted the wip/vitvakatu/doc-panel-redesign branch August 15, 2023 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation panel redesign #7372

Documentation panel redesign #7372

vitvakatu commented Jul 23, 2023

farmaazon Jul 27, 2023

MichaelMauderer Aug 1, 2023

kazcw Jul 27, 2023

kazcw Jul 27, 2023

MichaelMauderer Aug 1, 2023

kazcw Jul 27, 2023

kazcw Jul 27, 2023

kazcw Jul 27, 2023

kazcw Jul 27, 2023

kazcw Jul 27, 2023

somebody1234 left a comment

Frizi commented Aug 8, 2023 •

edited

Loading

vitvakatu commented Aug 8, 2023

vitvakatu commented Aug 9, 2023

xvcgreg commented Aug 11, 2023

Frizi commented Aug 11, 2023

	if key.to_lowercase() == "arguments" {
	if key.eq_ignore_ascii_case("Arguments") {

	self.current_list.push(mem::take(&mut self.current_body));
	self.current_list.push(self.current_body.drain(..).collect());

Documentation panel redesign #7372

Documentation panel redesign #7372

Conversation

vitvakatu commented Jul 23, 2023

Pull Request Description

Important Notes

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

somebody1234 left a comment

Choose a reason for hiding this comment

Frizi commented Aug 8, 2023 • edited Loading

QA Status: 🔴

vitvakatu commented Aug 8, 2023

vitvakatu commented Aug 9, 2023

xvcgreg commented Aug 11, 2023

Frizi commented Aug 11, 2023

QA Status: 🟡

Frizi commented Aug 8, 2023 •

edited

Loading