preferences: improve category headers and leaf #8512

liorfr · 2020-09-16T15:12:45Z

Fixes issue: #7745

Fixed category bug in the leaf. When the category contains more than one word the leaf contains category words.
Added colon after the subcategory in the leaf
Changed the size of the category and the subcategory

Before

After

How to test

Open the preferences

Signed-off-by: Lior Frenkel lior.frenkel@sap.com

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 · 2020-09-16T17:13:23Z

@colin-grant-work would you like to review the pull-request?

colin-grant-work · 2020-09-16T17:14:34Z

@vince-fugnitto, sure, I'll take a look later today.

packages/preferences/src/browser/views/preference-editor-widget.tsx

packages/preferences/src/browser/util/preference-tree-generator.ts

packages/preferences/src/browser/util/preference-tree-generator.spec.ts

liorfr

@colin-grant-work / @vince-fugnitto, Can you please check my changes?

colin-grant-work · 2020-09-17T14:29:49Z

It seems we're getting more instances of labels with colons than VSCode. Here's a side-by-side comparison of editor.find:

I think you may be treating the subcategory name as the leaf name, and the actual property name as the value in some cases.

liorfr

@colin-grant-work

The logic has not changed

Theia before my change

Theia after my change

We have two cases

Case 1: rawLeafValue exist:

Case 2: rawLeafValue does not exist:

The preferences behavior in Vs code is different than Theia however I believe that this PR is a step in the right direction. Can we merge this PR and define the new behavior in a new issue?

colin-grant-work · 2020-09-17T17:59:37Z

I agree your changes are an improvement, but I think we're pretty close to achieving the VSCode-type behavior and still may be able to in this PR:

if three parts (category.subcategory.property)
    use property as leaf name
else if two parts (category.property)
    use property as leaf name

Are there cases that I'm missing where we actually want to include more than the last part as the property name? If not, we can always just use the last segment, and discard the rest.

vince-fugnitto · 2020-09-17T17:59:40Z

The preferences behavior in Vs code is different than Theia however I believe that this PR is a step in the right direction. Can we merge this PR and define the new behavior in a new issue?

We can of course have an iterative approach to fix the headers if it currently adds value.
However, I don't believe the pull-request as is fixes #7745 (as we want a similar behavior as vscode).

Signed-off-by: Lior Frenkel <lior.frenkel@sap.com>

liorfr

@colin-grant-work, Can you please check my changes?

colin-grant-work · 2020-09-17T20:44:20Z

packages/preferences/src/browser/util/preference-tree-generator.ts

-        const propertySpecifier = this.split(property).slice(1);
-        const name = propertySpecifier.map(word => word.slice(0, 1).toLocaleUpperCase() + word.slice(1)).join(' ').trim();
+        const rawLeaf = property.split('.').pop();
+        const name = this.formatString(rawLeaf!);


Looks good to me. @vince-fugnitto, I'm a bit wary of the non-null assertion, but I've seen it elsewhere around the repo, and this seems like a fairly safe instance to me. What are your thoughts?

An option to avoid it would be to have .formatString return an empty string when passed undefined.

We could also do the following @colin-grant-work?

const rawLeaf: string = property.split('.').pop() || property; const name = this.formatString(rawLeaf);

It would be a safeguard against preferences which do not follow the format of being a.b but rather a.

I think it's functionally equivalent to the current code, but it would make Typescript happier 😄 . If there's no ., then the split will just return an array with the whole string at index 0.

I think it's functionally equivalent to the current code, but it would make Typescript happier . If there's no ., then the split will just return an array with the whole string at index 0.

Correct :) if you are wary about uses of it we can think about adding an eslint rule against it: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-non-null-assertion.md. I believe it would benefit the project to be more strict regarding null-checking.

colin-grant-work · 2020-09-17T20:45:01Z

@liorfr, the latest changes look good to me, and very close to VSCode. 👍
(Latest version, for comparison with previous)

vince-fugnitto

The changes look good to me 👍
Thank you for your contribution and for also adding tests!

liorfr

@vince-fugnitto, can you please merge?

vince-fugnitto · 2020-09-21T12:27:10Z

@vince-fugnitto, can you please merge?

Sure, I just wanted to give it a couple of days to give others the opportunity to review if they were interested.
I'll merge tomorrow morning if there are no objections.

colin-grant-work · 2020-09-21T16:50:34Z

This is likely outside of the scope of this PR, but I did notice today a couple of cases where the VSCode label includes a colon and consists of more than the final entry outside of the 'Commonly Used' section:

I'm not sure why 'Comments' isn't treated as a full subcategory here. There may be some number of children that a subcategory has to have to receive a full sub-head. I'm fine with the treatment of this type of situation in this PR.

vince-fugnitto · 2020-09-22T13:44:12Z

This is likely outside of the scope of this PR, but I did notice today a couple of cases where the VSCode label includes a colon and consists of more than the final entry outside of the 'Commonly Used' section:

I'm not sure why 'Comments' isn't treated as a full subcategory here. There may be some number of children that a subcategory has to have to receive a full sub-head. I'm fine with the treatment of this type of situation in this PR.

I think it is something we can fix at a later point, this pull-request is already a step in the right direction 👍

vince-fugnitto added preferences issues related to preferences ui/ux issues related to user interface / user experience labels Sep 16, 2020

vince-fugnitto reviewed Sep 16, 2020

View reviewed changes

packages/preferences/src/browser/views/preference-editor-widget.tsx Outdated Show resolved Hide resolved

colin-grant-work reviewed Sep 16, 2020

View reviewed changes

liorfr commented Sep 17, 2020

View reviewed changes

preferences: improve category headers and leaf

6d983e0

Signed-off-by: Lior Frenkel <lior.frenkel@sap.com>

liorfr commented Sep 17, 2020

View reviewed changes

colin-grant-work reviewed Sep 17, 2020

View reviewed changes

vince-fugnitto approved these changes Sep 17, 2020

View reviewed changes

liorfr commented Sep 21, 2020

View reviewed changes

vince-fugnitto merged commit 2d86390 into eclipse-theia:master Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preferences: improve category headers and leaf #8512

preferences: improve category headers and leaf #8512

liorfr commented Sep 16, 2020 •

edited

Loading

vince-fugnitto commented Sep 16, 2020

colin-grant-work commented Sep 16, 2020

liorfr left a comment

colin-grant-work commented Sep 17, 2020

liorfr left a comment

colin-grant-work commented Sep 17, 2020

vince-fugnitto commented Sep 17, 2020 •

edited

Loading

liorfr left a comment

colin-grant-work Sep 17, 2020

vince-fugnitto Sep 17, 2020

colin-grant-work Sep 17, 2020

vince-fugnitto Sep 17, 2020

colin-grant-work commented Sep 17, 2020 •

edited

Loading

vince-fugnitto left a comment

liorfr left a comment

vince-fugnitto commented Sep 21, 2020

colin-grant-work commented Sep 21, 2020

vince-fugnitto commented Sep 22, 2020

preferences: improve category headers and leaf #8512

preferences: improve category headers and leaf #8512

Conversation

liorfr commented Sep 16, 2020 • edited Loading

How to test

Review checklist

Reminder for reviewers

vince-fugnitto commented Sep 16, 2020

colin-grant-work commented Sep 16, 2020

liorfr left a comment

Choose a reason for hiding this comment

colin-grant-work commented Sep 17, 2020

liorfr left a comment

Choose a reason for hiding this comment

colin-grant-work commented Sep 17, 2020

vince-fugnitto commented Sep 17, 2020 • edited Loading

liorfr left a comment

Choose a reason for hiding this comment

colin-grant-work Sep 17, 2020

Choose a reason for hiding this comment

vince-fugnitto Sep 17, 2020

Choose a reason for hiding this comment

colin-grant-work Sep 17, 2020

Choose a reason for hiding this comment

vince-fugnitto Sep 17, 2020

Choose a reason for hiding this comment

colin-grant-work commented Sep 17, 2020 • edited Loading

vince-fugnitto left a comment

Choose a reason for hiding this comment

liorfr left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Sep 21, 2020

colin-grant-work commented Sep 21, 2020

vince-fugnitto commented Sep 22, 2020

liorfr commented Sep 16, 2020 •

edited

Loading

vince-fugnitto commented Sep 17, 2020 •

edited

Loading

colin-grant-work commented Sep 17, 2020 •

edited

Loading