vsx-extensions-view-styling-improvements #8086

kenneth-marut-work · 2020-06-24T18:29:18Z

What it does

This PR introduces some styling improvements to the vsx-extensions view to be more inline with VSCodes respective view, these improvements include:

Sticky "header" during scroll
Dividing border lines under header, as well as extension title in body
Max-width container for body content
Image resize on width resize
Repositioned scrollbar to be underneath header

How to test

Pull branch
Open extensions view
Search for extensions and double click to view content
Observe styling changes

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

vince-fugnitto

The changes look much better with a sticky header 👍

My only comment would be to preserve the same width for the custom scrollbar, with these changes (compared to master) the scrollbar is quite thin.

Pull Request:

Master:

kenneth-marut-work · 2020-06-24T20:26:20Z

@vince-fugnitto thanks for looking this over, I've just pushed up a slight change that fixes the scrollbar issue:

vince-fugnitto

The changes look very good! 👍
The next improvement would be to start the scrollbar right after the sticky header (similarly to vscode), but I think it can be done afterwards, the changes already are a big improvement.

kenneth-marut-work · 2020-06-24T21:37:39Z

Thanks @vince-fugnitto, I agree with you on the scrollbar improvement😎. I also noticed the quick scroll-to-top icon on the bottom right that could be added as well.

vince-fugnitto · 2020-06-24T21:44:39Z

Thanks @vince-fugnitto, I agree with you on the scrollbar improvement😎. I also noticed the quick scroll-to-top icon on the bottom right that could be added as well.

@kenneth-marut-work do you think the scroll issue should be included in the pull-request (since it is introduced with the sticky header)? The icon to scroll-to-top would be a subsequent improvement which is out of scope at the moment.

kenneth-marut-work · 2020-06-24T21:46:58Z

@vince-fugnitto Sounds good to me, I can certainly make that change in this PR 👍

kenneth-marut-work · 2020-06-25T16:59:42Z

@vince-fugnitto I've moved the location of the scrollbar to under the header, it required a couple of modifications to the widget and PS container, but it's working well on my machine in both browser and electron. Here's a screencast:

@colin-grant-work thanks for the help on this!

vince-fugnitto

Functionality the changes look very good 👍
I'll give others the chance to review as well :)

lmcbout · 2020-06-25T19:26:13Z

Sticky header whne scrolling is nice.
I have two remars:
1- When re-sizing smaller, ther is no Horizontal scroll for the sticky portion
2- The bottom part is wrapping except when there is a comment in the text.
Check the text is wrapping except the comment line with "... present Microsoft..."

kenneth-marut-work · 2020-06-25T19:59:37Z

Hi @lmcbout
Thanks for the your comments. Quick question about the following:

1- When re-sizing smaller, ther is no Horizontal scroll for the sticky portion

I do not see this behavior in VSCode, is this still something we would like to implement? See VSCode behavior below:

vince-fugnitto · 2020-06-25T20:14:12Z

Hi @lmcbout
Thanks for the your comments. Quick question about the following:

1- When re-sizing smaller, ther is no Horizontal scroll for the sticky portion

I do not see this behavior in VSCode, is this still something we would like to implement? See VSCode behavior below:

@kenneth-marut-work no I wouldn’t add a scrollbar (neither horizontal or vertical) for the sticky header, it does not align with the rest of the framework nor with vscode.

kenneth-marut-work · 2020-06-25T20:17:33Z

@vince-fugnitto thanks for the response, in that case I have pushed the adjustment regarding @lmcbout 's comment:

2- The bottom part is wrapping except when there is a comment in the text.
Check the text is wrapping except the comment line with "... present Microsoft..."

The change targets pre elements inside the HTML body and adds text wrapping:

lmcbout

LGTM
Concerning item 1

no I wouldn’t add a scrollbar (neither horizontal or vertical) for the sticky header, it does not align with the rest of the framework nor with vscode.

When the window width is narrow, we don't see any content on the right side. I don't think it is user friendly. I understand it is the same behavior on VSCode, but still :(
Lets keep it as it is right now and we will see if we get other comments about it later

Good jod @kenneth-marut-work

akosyakov · 2020-06-27T07:22:34Z

packages/vsx-registry/src/browser/style/index.css

@@ -17,6 +17,7 @@
 :root {
  --theia-vsx-extension-icon-size: calc(var(--theia-ui-icon-font-size)*3);
  --theia-vsx-extension-editor-icon-size: calc(var(--theia-vsx-extension-icon-size)*3);
+  --theia-vsx-extension-header-height: 150px;


how do you get 150? Would it still work if some font size css variable is redefined? Could it be expressed as a formula?

Thanks for the comment @akosyakov
I had originally used this value since VSCode seems to have a fixed header size of 166px but also includes a tabbar (which we do not have). I have removed this hard coded value and instead calculated the heights using JS from the deferred HTMLElements inside vsx-extension-editor.tsx .

akosyakov · 2020-06-27T07:26:21Z

packages/vsx-registry/src/browser/vsx-extension-editor.tsx

        this.update();
        this.toDispose.push(this.model.onDidChange(() => this.update()));
    }

+    async getScrollContainer(): Promise<HTMLElement> {
+        await waitForRevealed(this);


looks fragile, one should rather pass deferred promise into the rect component and resolve in ref assignment

@akosyakov Thanks for the suggestion, I did not know about deferred technique but went ahead and implemented it according to my understanding

akosyakov · 2020-06-30T07:10:18Z

packages/vsx-registry/src/browser/vsx-extension.tsx

        } = this.props.extension;
        return <React.Fragment>
-            <div className='header'>
+            <div className='header' ref={element => this.resolveDeferredOnRender(element, deferredHeaderElementRender)}>


you should just update the property of this class without any deferred the same in other places

I've updated this in my latest changes. The only remaining deferred item is the scroll container which is used for getScrollContainer inside vsx-extension-editor

akosyakov · 2020-06-30T07:11:46Z

packages/vsx-registry/src/browser/vsx-extension.tsx

@@ -99,6 +100,9 @@ export class VSXExtension implements VSXExtensionData, TreeElement {
    readonly search: VSXExtensionsSearchModel;

    protected readonly data: Partial<VSXExtensionData> = {};
+    deferredScrollContainerRender: Deferred<HTMLDivElement> | undefined;


please don't break encapsulation, clients should be able to modify rendered DOM elements

protected _header: HTMLDivElement | undefined; get header(): HTMLDIvElement | undefined { return this._header; }

the same for other properties

Thanks for the suggestions, I've reorganized the logic so that these elements remain encapsulated. I have also added a method to reset the vsx-extension.tsx deferred scrollContainer property during tab close/reopen since the previously opened instance will persist through a close/reopen

akosyakov · 2020-06-30T07:15:34Z

packages/vsx-registry/src/browser/vsx-extension-editor.tsx

+        super.onResize(msg);
+        const bodyElement = await this.deferredBodyElementRender.promise;
+        const scrollContainer = await this.deferredScrollContainerRender.promise;
+        if (bodyElement && scrollContainer) {


it does not look good, since we modify rendered DOM than we should touch only virtual, we should propagate onResize to VSXExtension and there update new sizes as part of the state to retrigger DOM update via React

akosyakov · 2020-07-02T06:34:12Z

packages/vsx-registry/src/browser/vsx-extension.tsx

@@ -100,6 +104,56 @@ export class VSXExtension implements VSXExtensionData, TreeElement {

    protected readonly data: Partial<VSXExtensionData> = {};

+    protected _body: HTMLDivElement | undefined;


It should be replaced with:

protected _editor: VSXExtensionEditorComponent | undefined; renderEditor(): React.ReactNode { return <VSXExtensionEditorComponent extension={this} ref={ref = (this._editor = ref || undefined)} />; }

Hi @akosyakov, the reason I have these additional (body, header, and scrollContainer) elements is because it seems that the absence of a readme on line 475 could potentially cause the body and scroll-container to not be rendered. Additionally because there is manipulation of the virtual DOM and I need a ref to all 3 elements in setContainerHeights and updateOnResize to keep track of each individually.

My problem is not that you add them, but how, i.e. there are public APIs to allow setting them by child components. Instead proper encapsulation should be applied, i.e. they should be exposed on child components instead and here we should only capture a child component ref::

const body = this._editor?.body;

akosyakov · 2020-07-02T06:34:44Z

packages/vsx-registry/src/browser/vsx-extension.tsx

+    protected header?: HTMLDivElement;
+    protected body?: HTMLDivElement;
+
+    componentDidMount(): void {


you don't need all of these

Since I am keeping track of these in VSXExtension, I went ahead and removed these redundancies as well as some of the checks inside componentDidMount. The refs are now set using the setter methods passed in as props and to VSXExtensionEditorComponent, and componentDidMount will trigger the initial header/body sizing calculations if the refs exist

akosyakov · 2020-07-05T05:32:55Z

packages/vsx-registry/src/browser/vsx-extension.tsx

    // TODO replace with webview
    readonly openLink = (event: React.MouseEvent) => {
-        if (!this.body) {
+        if (!this.props.extension.body) {


body ref should belong to this component, not to its parent, please don't break encapsulation

kenneth-marut-work · 2020-07-07T17:37:14Z

@akosyakov Thanks for the comments, I've moved all refs used for sizing calculations to VSXExtensionEditorComponent for proper encapsulation. The VSXExtensionEditor widget's onResize width is propagated to VSXExtension and passed into VSXExtensionEditorComponent as a prop, which triggers a re-render and size calculations which are methods and properties confined to VSXExtensionEditorComponent.

The VSXExtensionEditorComponent's react life-cycle methods are now responsible for resolving and resetting the deferred scrollContainer which are needed by the VSXExtensionEditor widget:

theia/packages/vsx-registry/src/browser/vsx-extension.tsx

Line 386 in 680deac

this.props.extension.resolveDeferredScrollContainer(this.scrollContainer);

Please note I am aware that this breaks encapsulation of the VSXExtensionEditorComponent's scrollContainer member, but seems to be the only way to pass the value upward to the getScrollContainer method since there is currently no way to reference an exposed getter on the instance of VSXExtensionEditorComponent as it is hardcoded inside VSXExtension:

theia/packages/vsx-registry/src/browser/vsx-extension.tsx

Line 291 in 680deac

return <VSXExtensionEditorComponent extension={this} />;

Also note build seems to be failing due to:
error /Users/travis/build/eclipse-theia/theia/node_modules/vscode-ripgrep: Command failed.

akosyakov · 2020-07-08T07:02:05Z

packages/vsx-registry/src/browser/vsx-extension.tsx

        } = this.props.extension;
+
+        if (width !== undefined) {


Why is it happening on render but no on proper events like resize?

akosyakov · 2020-07-08T07:05:11Z

packages/vsx-registry/src/browser/vsx-extension.tsx

+        return this._width;
+    }
+    set width(width: number) {
+        this._width = width;


On resitze event, you should call VSXExtensionEditorComponent.setState and keep in the state everything what is required for rendering. render function should not compute any new properties but simply use state and props to render. That's how react works.

colin-grant-work · 2020-07-08T16:50:04Z

@akosyakov, @kenneth-marut-work, reading over a number of the comments on this PR, my impression is that a lot of the discussion has revolved around questions of encapsulation and the proper location for various member variables. It seems to me that origin of a lot of this difficulty is that the existing implementation has made the VSXExtension class serve two purposes: rendering a TreeNode in the extensions list widget and rendering the VSXExtensionEditorComponent. It isn't really fit for both purposes, and it shouldn't be asked to do both jobs.

When the VSXExtension is acting as a TreeElement, resizing etc. is handled by the TreeWidget, and because that logic is handled by the TreeWidget, the VSXExtension doesn't need to handle it. When the VSXEditor is rendering the VSXExtensionEditorComponent, analogy would dictate that the resizing and other state logic should be handled by the widget that is ultimately rendering the editor, VSXExtensionEditor, not by VSXExtension. Trying to handle events in the VSXExtension turns it into a props object that renders itself - which is definitely not how React is supposed to work. As a props object, the VSXExtension can't inform its renderer of changes to itself: it isn't set up to be a widget, able to call update on a state change. It's set up to house static data and render that data in the limited context of a TreeWidget.

Instead, I think Ken's instinct was correct to track the scroll container and other state relevant to the VSXExtensionEditorComponent in the VSXExtensionEditor widget. That isn't breaking encapsulation; it's keeping state data about a widget on that widget. To make that division of labor clearer, I'd propose removing the .renderEditor() method from VSXExtension and replacing the .render() method of VSXExtensionEditor with one that returns <VSXExtensionEditorComponent extension={this.extension} {{...andMaybeOtherProps}} />. That way the VSXExtension class retains its role as a repository of data about a particular extension, and the VSXExtensionEditor widget can handle widget-related state and events.

Does that sound like a reasonable adjustment to the current architecture?

akosyakov · 2020-07-09T08:10:53Z

Moving VSXExtensionEditorComponent sounds fine with me. But my issues were not understanding the flow: like state is kept not in the component but somewhere else, child components methods of parent components and so on. Moving component does not fix it.

colin-grant-work · 2020-07-09T15:48:38Z

@akosyakov, I've pushed a new version that moves the call to VSXExtensionEditorComponent into the VSXExtensionEditor, and I believed it has obviated many of the problems you described. The reason that the VSXExtensionEditorComponent was (undesirably) given access to methods on the VSXExtension was because of a mismatch between the lifecycle of the VSXExtension - persistent - and the VSXExtensionEditor widget - able to be opened and closed at will. As long as the VSXExtensionEditorComponent was housed in the VSXExtension, it had to enforce the lifecycle of a widget (the VSXExtensionEditor) on something that was not a widget (the VSXExtension) by registering its mounting and unmounting events.

Moving the VSXExtensionEditorComponent into the VSXExtensionEditor widget class removes that mismatch in lifecycles and means that the VSXExtensionEditor widget can simply manage its own lifecycle and use the VSXExtensionEditorComponent as a renderer, and the VSXExtension doesn't have to worry about it at all.

I've also moved some of the styling logic - the max-width and margin of the body subcomponent - into CSS, but if you prefer that those calculations be made on the JS side, I'm happy to revert that part.

akosyakov

thanks, code wise it looks very clean now. Someone has to test it.

packages/vsx-registry/src/browser/vsx-extension-editor.tsx

Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com> Signed-off-by: Colin Grant <colin.grant@ericsson.com>

colin-grant-work · 2020-07-14T13:37:18Z

@vince-fugnitto, would you be willing to give this a quick once over to make sure everything is still working after these latest changes? the CI checks are failing at the moment, but it looks like an error downloading rip-grep in one build, rather than a problem with the code.

vince-fugnitto · 2020-07-14T13:40:29Z

@vince-fugnitto, would you be willing to give this a quick once over to make sure everything is still working after these latest changes? the CI checks are failing at the moment, but it looks like an error downloading rip-grep in one build, rather than a problem with the code.

Of course :) I'll also restart the build (ripgrep issues occur for forked repositories since they do not have the necessary tokens).

vince-fugnitto

The changes look very good 👍
I've verified that:

the header is sticky
the scrollbar works correctly (and starts after the header)
the view is responsive (content wraps correctly, images are resized properly)

vince-fugnitto · 2020-07-15T00:15:07Z

I'll merge tomorrow if there are no objections :)

kenneth-marut-work changed the title ~~feature/extensions-view-styling-improvements~~ WIP: feature/extensions-view-styling-improvements Jun 24, 2020

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch from d4a2b84 to 005516f Compare June 24, 2020 18:41

kenneth-marut-work changed the title ~~WIP: feature/extensions-view-styling-improvements~~ vsx-extensions-view-styling-improvements Jun 24, 2020

vince-fugnitto reviewed Jun 24, 2020

View reviewed changes

vince-fugnitto added ui/ux issues related to user interface / user experience vsx-registry Issues related to Open VSX Registry Integration labels Jun 24, 2020

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch from 005516f to 5f2c6d4 Compare June 24, 2020 20:19

vince-fugnitto approved these changes Jun 24, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch from 5f2c6d4 to cb28fe6 Compare June 25, 2020 16:36

vince-fugnitto requested a review from lmcbout June 25, 2020 18:46

vince-fugnitto approved these changes Jun 25, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch from cb28fe6 to 9bc1c82 Compare June 25, 2020 20:11

lmcbout approved these changes Jun 26, 2020

View reviewed changes

akosyakov reviewed Jun 27, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch 2 times, most recently from 8d17032 to 6f92fff Compare June 29, 2020 17:57

akosyakov reviewed Jun 30, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch from 6f92fff to d5c3701 Compare July 1, 2020 21:18

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch 3 times, most recently from 99b8480 to eb9cbef Compare July 1, 2020 21:46

akosyakov reviewed Jul 2, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch from eb9cbef to e5c6296 Compare July 2, 2020 14:57

akosyakov reviewed Jul 5, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/extensions-styling-improvements branch 2 times, most recently from 680deac to 0dd4d84 Compare July 7, 2020 17:01

akosyakov reviewed Jul 8, 2020

View reviewed changes

colin-grant-work force-pushed the feature/extensions-styling-improvements branch 2 times, most recently from 22f2d78 to 37d7602 Compare July 9, 2020 15:29

akosyakov approved these changes Jul 10, 2020

View reviewed changes

packages/vsx-registry/src/browser/vsx-extension-editor.tsx Outdated Show resolved Hide resolved

packages/vsx-registry/src/browser/vsx-extension-editor.tsx Outdated Show resolved Hide resolved

extensions styling improvements

1a63d2d

Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com> Signed-off-by: Colin Grant <colin.grant@ericsson.com>

colin-grant-work force-pushed the feature/extensions-styling-improvements branch from 37d7602 to 1a63d2d Compare July 12, 2020 20:05

vince-fugnitto approved these changes Jul 14, 2020

View reviewed changes

vince-fugnitto merged commit 349fd06 into eclipse-theia:master Jul 16, 2020

vince-fugnitto mentioned this pull request Aug 27, 2020

vsx: update display of an extension's readme #7369

Closed

		@@ -100,6 +104,56 @@ export class VSXExtension implements VSXExtensionData, TreeElement {

		protected readonly data: Partial<VSXExtensionData> = {};

		protected _body: HTMLDivElement \| undefined;

vsx-extensions-view-styling-improvements #8086

vsx-extensions-view-styling-improvements #8086

Conversation

kenneth-marut-work commented Jun 24, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Jun 24, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Jun 24, 2020

vince-fugnitto commented Jun 24, 2020

kenneth-marut-work commented Jun 24, 2020

kenneth-marut-work commented Jun 25, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

lmcbout commented Jun 25, 2020

kenneth-marut-work commented Jun 25, 2020

vince-fugnitto commented Jun 25, 2020

kenneth-marut-work commented Jun 25, 2020

lmcbout left a comment

Choose a reason for hiding this comment

akosyakov Jun 27, 2020 • edited Loading

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

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

kenneth-marut-work commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work commented Jul 8, 2020 • edited Loading

akosyakov commented Jul 9, 2020

colin-grant-work commented Jul 9, 2020 • edited Loading

akosyakov left a comment

Choose a reason for hiding this comment

colin-grant-work commented Jul 14, 2020

vince-fugnitto commented Jul 14, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Jul 15, 2020

kenneth-marut-work commented Jun 24, 2020 •

edited

Loading

akosyakov Jun 27, 2020 •

edited

Loading

colin-grant-work commented Jul 8, 2020 •

edited

Loading

colin-grant-work commented Jul 9, 2020 •

edited

Loading