Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Truncate extension descriptions #8282

Merged
merged 4 commits into from
Jul 22, 2014
Merged

Truncate extension descriptions #8282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
038a60f
Shorten extension descriptions to 140 characters, do not display keyw…
Jun 28, 2014
e1dc1bb
Merge remote-tracking branch 'upstream/master' into issue-8216
Jul 21, 2014
bbe17ff
Changes per review
Jul 21, 2014
2bd4f7b
Increase limit to 200 characters
Jul 22, 2014
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
27 changes: 26 additions & 1 deletion src/extensibility/ExtensionManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,31 @@ define(function (require, exports, module) {
}, []);
}

/**
* Toggles between truncated and full length extension descriptions
* @param {string} id The id of the extension clicked
* @param {JQueryElement} $element The DOM element of the extension clicked
* @param {boolean} showFull true if full length description should be shown, false for shorten version.
*/
function toggleDescription(id, $element, showFull) {
var description, linkTitle,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's usually one variable per line

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use more than one in he first line. JSLint allows you to do that. I often do that so that you don't get lines with just a variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unsure about this, so I checked and just double checked the coding conventions, and I found nothing about single/multiple variables per line. Should I leave it or initialize them as null or something?

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 is fine how it is now. Those are initialized to undefined by JavaScript anyway.

entry = extensions[id];

// Toggle between appropriate descriptions and link title,
// depending on if extension is installed or not
if (showFull) {
description = entry.installInfo ? entry.installInfo.metadata.description : entry.registryInfo.metadata.description;
linkTitle = Strings.VIEW_TRUNCATED_DESCRIPTION;
} else {
description = entry.installInfo ? entry.installInfo.metadata.shortdescription : entry.registryInfo.metadata.shortdescription;
linkTitle = Strings.VIEW_COMPLETE_DESCRIPTION;
}

$element.attr("data-toggle-desc", showFull ? "trunc-desc" : "expand-desc")
.attr("title", linkTitle)
.prev(".ext-full-description").html(description);
}

// Listen to extension load and loadFailed events
$(ExtensionLoader)
.on("load", _handleExtensionLoad)
Expand All @@ -600,7 +625,7 @@ define(function (require, exports, module) {
exports.updateExtensions = updateExtensions;
exports.getAvailableUpdates = getAvailableUpdates;
exports.cleanAvailableUpdates = cleanAvailableUpdates;

exports.toggleDescription = toggleDescription;
exports.ENABLED = ENABLED;
exports.START_FAILED = START_FAILED;

Expand Down
14 changes: 11 additions & 3 deletions src/extensibility/ExtensionManagerView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ define(function (require, exports, module) {
InstallExtensionDialog = require("extensibility/InstallExtensionDialog"),
LocalizationUtils = require("utils/LocalizationUtils"),
itemTemplate = require("text!htmlContent/extension-manager-view-item.html");

/**
* Creates a view enabling the user to install and manage extensions. Must be initialized
* with initialize(). When the view is closed, dispose() must be called.
Expand Down Expand Up @@ -110,7 +110,7 @@ define(function (require, exports, module) {
* The individual views for each item, keyed by the extension ID.
*/
ExtensionManagerView.prototype._itemViews = null;

/**
* @private
* Attaches our event handlers. We wait to do this until we've fully fetched the extension list.
Expand Down Expand Up @@ -154,6 +154,10 @@ define(function (require, exports, module) {
ExtensionManager.markForRemoval($target.attr("data-extension-id"), true);
} else if ($target.hasClass("undo-update")) {
ExtensionManager.removeUpdate($target.attr("data-extension-id"));
} else if ($target.attr("data-toggle-desc") === "expand-desc") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to let you know, jQuery has a .data() method to get the data attributes, which makes the code look a bit cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but all the other code in that same if-else-if block uses .attr() to access the data- attributes. So I guess the question of which one to use comes down to consistency with the rest of the code around it for better method.

Also, I can't seem to get .data() to work correctly. It will pick up the expand-desc value but not the trunc-desc value, so the description expands but never contracts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't saw the use of .attr() in this file. I've been using .data() for data attributes. Maybe to make it work you need to also add the data attributes with .data()? Anyway it was a minor nit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alrighty then. I'll leave it as-is.

ExtensionManager.toggleDescription($target.attr("data-extension-id"), $target, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$target needs to be passed in order to target just that extension's description.

} else if ($target.attr("data-toggle-desc") === "trunc-desc") {
ExtensionManager.toggleDescription($target.attr("data-extension-id"), $target, false);
}
})
.on("click", "button.install", function (e) {
Expand Down Expand Up @@ -206,7 +210,11 @@ define(function (require, exports, module) {
// (or registry is offline). These flags *should* always be ignored in that scenario, but just in case...
context.isCompatible = context.isCompatibleLatest = true;
}


if (info.metadata.description !== undefined) {
info.metadata.shortdescription = StringUtils.truncate(info.metadata.description, 200);
}

context.isMarkedForRemoval = ExtensionManager.isMarkedForRemoval(info.metadata.name);
context.isMarkedForUpdate = ExtensionManager.isMarkedForUpdate(info.metadata.name);

Expand Down
28 changes: 16 additions & 12 deletions src/htmlContent/extension-manager-view-item.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,25 @@
{{/isCompatibleLatest}}
{{/isCompatible}}
{{/showInstallButton}}
{{metadata.description}}
{{^metadata.description}}
<p class="muted"><em>{{Strings.EXTENSION_NO_DESCRIPTION}}</em></p>
{{/metadata.description}}
<span class="ext-full-description">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked up the Mustache syntax, and I think this how all this should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but maybe a some indentation will make the code look better like:

            {{#hasShortDescription}}
                {{metadata.shortdescription}}
            {{/hasShortDescription}}
            {{^hasShortDescription}}
                {{#metadata.description}}
                    {{metadata.description}}
                {{/metadata.description}}
                {{^metadata.description}}
                    <p class="muted"><em>{{Strings.EXTENSION_NO_DESCRIPTION}}</em></p>
                {{/metadata.description}}
            {{/hasShortDescription}}

{{#metadata.shortdescription}}
{{metadata.shortdescription}}
{{/metadata.shortdescription}}
{{^metadata.shortdescription}}
{{#metadata.description}}
{{metadata.description}}
{{/metadata.description}}
{{^metadata.description}}
<p class="muted"><em>{{Strings.EXTENSION_NO_DESCRIPTION}}</em></p>
{{/metadata.description}}
{{/metadata.shortdescription}}
</span>
{{#metadata.shortdescription}}
<a data-extension-id="{{metadata.name}}" data-toggle-desc="expand-desc" title="{{Strings.VIEW_COMPLETE_DESCRIPTION}}" href="#">...</a>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The link must remain outside .ext-full-description so it is not wiped away when the description is changed.

{{/metadata.shortdescription}}
{{#metadata.homepage}}
<p><a title="{{metadata.homepage}}" href="{{metadata.homepage}}">{{Strings.EXTENSION_MORE_INFO}}</a></p>
{{/metadata.homepage}}
{{#metadata.keywords.length}}
<br/>
<span class="ext-keywords">{{Strings.EXTENSION_KEYWORDS}}:
{{#metadata.keywords}}
{{.}}
{{/metadata.keywords}}
</span>
{{/metadata.keywords.length}}
{{#translated}}
<br/>
<span class="ext-translated" title="{{translatedLangs}}">{{extensionTranslated}}</span>
Expand Down
2 changes: 2 additions & 0 deletions src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,8 @@ define({
"CANCELING_INSTALL" : "Canceling\u2026",
"CANCELING_HUNG" : "Canceling the install is taking a long time. An internal error may have occurred.",
"INSTALL_CANCELED" : "Installation canceled.",
"VIEW_COMPLETE_DESCRIPTION" : "View complete description",
"VIEW_TRUNCATED_DESCRIPTION" : "View truncated description",
// These must match the error codes in ExtensionsDomain.Errors.* :
"INVALID_ZIP_FILE" : "The downloaded content is not a valid zip file.",
"INVALID_PACKAGE_JSON" : "The package.json file is not valid (error was: {0}).",
Expand Down
3 changes: 0 additions & 3 deletions src/styles/brackets_patterns_override.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,9 +972,6 @@ a[href^="http"] {
.user-select(text);
cursor: text;
}
.ext-keywords {
color: @tc-input-placeholder-text;
}
.ext-translated {
color: @tc-input-placeholder-text;
}
Expand Down
22 changes: 22 additions & 0 deletions src/utils/StringUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,27 @@ define(function (require, exports, module) {
return returnVal;
}

/**
* Truncate strings to specified length.
* @param {string} str Text to be truncated.
* @param {number} len Length to which text should be limited.
* @return {string} Returns truncated text only if it was changed.
*/
function truncate(str, len) {
// Truncate the description if it is too long
if (str.length > len) {
str = str.substr(0, len);

// To prevent awkward addition of ellipsis, try to truncate
// at the end of the last whole word
var lastSpaceChar = str.lastIndexOf(" ");
if (lastSpaceChar < len && lastSpaceChar > -1) {
str = str.substr(0, lastSpaceChar);
}
return str;
}
}

// Define public API
exports.format = format;
exports.htmlEscape = htmlEscape;
Expand All @@ -209,4 +230,5 @@ define(function (require, exports, module) {
exports.breakableUrl = breakableUrl;
exports.endsWith = endsWith;
exports.prettyPrintBytes = prettyPrintBytes;
exports.truncate = truncate;
});
32 changes: 29 additions & 3 deletions test/spec/ExtensionManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,8 +770,7 @@ define(function (require, exports, module) {

// Simple fields
[item.metadata.version,
item.metadata.author && item.metadata.author.name,
item.metadata.description]
item.metadata.author && item.metadata.author.name]
.forEach(function (value) {
if (value) {
expect(view).toHaveText(value);
Expand All @@ -782,7 +781,7 @@ define(function (require, exports, module) {
}

// Array-valued fields
[item.metadata.keywords, item.metadata.categories].forEach(function (arr) {
[item.metadata.categories].forEach(function (arr) {
if (arr) {
arr.forEach(function (value) {
expect(view).toHaveText(value);
Expand All @@ -793,6 +792,33 @@ define(function (require, exports, module) {
});
});

it("should display original description", function () {
setupViewWithMockData(ExtensionManagerViewModel.RegistryViewModel);
runs(function () {
_.forEach(mockRegistry, function (item) {
if (item.metadata.description) {
if (StringUtils.truncate(item.metadata.description, 200) === undefined) {
expect(view).toHaveText(item.metadata.description);
}
}
});
});
});

it("should display shortened description", function () {
setupViewWithMockData(ExtensionManagerViewModel.RegistryViewModel);
runs(function () {
_.forEach(mockRegistry, function (item) {
if (item.metadata.description) {
var shortDescription = StringUtils.truncate(item.metadata.description, 200);
if (shortDescription !== undefined) {
expect(view).toHaveText(shortDescription);
}
}
});
});
});

it("should display owner even for installed items", function () {
ExtensionManager._setExtensions(JSON.parse(mockExtensionList));
setupViewWithMockData(ExtensionManagerViewModel.InstalledViewModel);
Expand Down