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.

Sign up for GitHub

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

preferences: improve category headers and leaf #8512

Merged
merged 1 commit into from
Sep 22, 2020
Merged

preferences: improve category headers and leaf #8512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/preferences/src/browser/style/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,13 +189,17 @@
text-decoration: underline;
}



.theia-settings-container .settings-section-title {
.theia-settings-container .settings-section-category-title {
font-weight: bold;
font-size: var(--theia-ui-font-size3);
padding-left: calc(2 * var(--theia-ui-padding));
}
}

.theia-settings-container .settings-section-subcategory-title {
font-weight: bold;
font-size: var(--theia-ui-font-size2);
padding-left: calc(2 * var(--theia-ui-padding));
}

.theia-settings-container .settings-section>li {
list-style-type: none;
Expand Down
35 changes: 35 additions & 0 deletions packages/preferences/src/browser/util/preference-tree-generator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { Container } from 'inversify';
import { PreferenceTreeGenerator } from './preference-tree-generator';
import { PreferenceSchemaProvider } from '@theia/core/lib/browser';
import { PreferenceConfigurations } from '@theia/core/lib/browser/preferences/preference-configurations';
import { Preference } from './preference-types';

disableJSDOM();

Expand All @@ -51,4 +52,38 @@ describe('preference-tree-generator', () => {
expect(preferenceTreeGenerator['split'](testString)).deep.eq(testString.split(splitter));
});

it('PreferenceTreeGenerator.format', () => {
const testString = 'aaaBbbCcc Dddd eee';
expect(preferenceTreeGenerator['formatString'](testString)).eq('Aaa Bbb Ccc Dddd Eee');
});

describe('PreferenceTreeGenerator.createLeafNode', () => {
vince-fugnitto marked this conversation as resolved.
Show resolved Hide resolved
it('when property constructs of three parts the third part is the leaf', () => {
const property = 'category-name.subcategory.leaf';
const expectedName = 'Leaf';
testLeafName(property, expectedName);
});

it('when property constructs of two parts the second part is the leaf', () => {
const property = 'category-name.leaf';
const expectedName = 'Leaf';
testLeafName(property, expectedName);
});

function testLeafName(property: string, expectedName: string): void {
const preferencesGroups: Preference.Branch[] = [];
const root = preferenceTreeGenerator['createRootNode'](preferencesGroups);
const preferencesGroup = preferenceTreeGenerator['createPreferencesGroup']('group', root);

const expectedSelectableTreeNode = {
id: property,
name: expectedName,
parent: preferencesGroup,
visible: true,
selected: false,
};
expect(preferenceTreeGenerator['createLeafNode'](property, preferencesGroup)).deep.eq(expectedSelectableTreeNode);
}

});
});
8 changes: 6 additions & 2 deletions packages/preferences/src/browser/util/preference-tree-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ export class PreferenceTreeGenerator {
});

protected createLeafNode = (property: string, preferencesGroup: Preference.Branch): SelectableTreeNode => {
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!);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

return {
id: property,
name,
Expand Down Expand Up @@ -119,4 +119,8 @@ export class PreferenceTreeGenerator {
return split;
}

private formatString(string: string): string {
const specifier = this.split(string);
return specifier.map(word => word.slice(0, 1).toLocaleUpperCase() + word.slice(1)).join(' ').trim();
}
}
4 changes: 3 additions & 1 deletion packages/preferences/src/browser/views/preference-editor-widget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,15 @@ export class PreferencesEditorWidget extends ReactWidget {

protected renderCategory(category: Preference.Branch): React.ReactNode {
const children = category.children.concat(category.leaves).sort((a, b) => this.sort(a.id, b.id));
const isCategory = category.parent?.parent === undefined;
const categoryLevelClass = isCategory ? 'settings-section-category-title' : 'settings-section-subcategory-title';
return category.visible && (
<ul
className="settings-section"
key={`${category.id}-editor`}
id={`${category.id}-editor`}
>
<li className="settings-section-title" data-id={category.id}>{category.name}</li>
<li className={categoryLevelClass} data-id={category.id}>{category.name}</li>
{children.map((preferenceNode: SelectableTreeNode | Preference.Branch) => {
if (Preference.Branch.is(preferenceNode)) {
return this.renderCategory(preferenceNode);
Expand Down