Truncate extension descriptions #8282

le717 · 2014-06-29T20:55:30Z

This is for #8216.

Code to toggle between original and truncated description
Visual review
Code review

le717 · 2014-06-30T04:44:26Z

Quick update: I have been working on displaying the full extension upon click, the trick has been changing only that description and not all of them. Once I get that, then I can work on toggling between the two. I'm sure I'll get it working very soon.

le717 · 2014-06-30T20:55:13Z

@larz0 I've got the toggle actions you requested implemented, if you would want to look at it. I doubt this is the final code implementation, but that should not effect the look. I made a quick GIF overview for you too. :)

le717 · 2014-06-30T20:56:17Z

src/extensibility/ExtensionManager.js

+            description = entry.installInfo ? entry.installInfo.metadata.shortdescription : entry.registryInfo.metadata.shortdescription;
+        }
+
+        $(element).toggleClass("expand-desc trunc-desc");


This was the only way I could figure out how to trigger the appropriate hide/show actions.

larz0 · 2014-06-30T21:27:36Z

Cool. Looks good! Thanks for the gif.

marcelgerber · 2014-07-01T16:23:28Z

src/extensibility/ExtensionManagerView.js

+
+                // To prevent awkward addition of ellipsis, try to truncate the description
+                // at the end of the last whole word
+                if (description.lastIndexOf(" ") < descriptionLimit && description.lastIndexOf(" ") > -1) {


It's better to set a var to description.lastIndexOf(" ") and use it instead of calling the function 3 times.

le717 · 2014-07-01T17:12:44Z

@SAplayer Changes pushed. Do you still want me to move the truncate function to StringUtils for more general-purpose usage? The way I had to access the function in test/spec/ExtensionManager-test.js feels a bit hacky to me.

marcelgerber · 2014-07-01T19:03:06Z

src/extensibility/ExtensionManager.js

+     * @param {boolean} short true if short description should be shown, false for full length.
+     */
+    function toggleDescription(id, element, short) {
+        var description, linkTitle,


It's usually one variable per line

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.

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?

I think it is fine how it is now. Those are initialized to undefined by JavaScript anyway.

le717 · 2014-07-01T20:17:41Z

Changes pushed, except for the toggleClass second parameter question. :)

TomMalbran · 2014-07-01T20:21:42Z

src/extensibility/ExtensionManager.js

+            linkTitle = Strings.VIEW_TRUNCATED_DESCRIPTION;
+        }
+
+        $element.toggleClass("expand-desc trunc-desc")


The old comment is gone. So i'll reply here.

You are not really using the classes to style the element, so maybe you shouldn't use a class. To store data in the DOM use the data attributes. You can just have a single data-* attribute and change i'ts value.

Or if that doesn't work, you can use 2 toggles.

The data-* method works. Now what to show it? data-toggle-desc?

I don't know. I didn't wrote because I couldn't think of a good name for it.

le717 · 2014-07-01T21:17:24Z

Changes pushed. I ended up calling it data-toggle-desc because it makes sense to what it does, at least to me.

TomMalbran · 2014-07-01T21:19:41Z

src/extensibility/ExtensionManagerView.js

@@ -156,6 +189,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") {


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

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.

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.

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

le717 · 2014-07-02T04:27:47Z

Thanks for reviewing this, everyone. The only questions I have now are

If ExtensionManagerView._truncateDescription() should be slightly refactored to have the limit set via argument and move it to StringUtils (as @SAplayer mentioned)
Do need to be unit tests for the short/full description displaying?

That first one is important, as right now in order to fix the current unit test for description view (test/spec/ExtensionManager-test.js, line 776) makes a new instance of ExtensionManagerView just to access the function. I feel funny about doing that, not to mention it is probably not the most ideal thing in the world, considering all that ExtensionManagerView does.

TomMalbran · 2014-07-02T04:40:15Z

@le717 If you want you can do that but since no other module uses it, I am not sure if is required.

My concern is about having the description duplicated for almost every extension, since shortDescription is actually the description for almost all the extensions. With that said, hasShortDescription has kind of the wrong meaning if you don't read how it is implemented, since every extension has a shortDescription and you want to expand the ones where the short description is actually shorter than the description. But I am not quite sure what is the best solution.

le717 · 2014-07-02T17:45:32Z

I'm just thinking about efficiency and good practice, as ExtensionManagerView is what initializes the entire registry manager... I'll leave it there for now, and maybe someone from the Adobe team will know what to do.

I could change it to where it shortened the description, check if it is different, and only if it is different will it be added to the extension details. This way, for most of the extensions only one copy of the description will be present.

le717 · 2014-07-07T16:15:58Z

src/htmlContent/extension-manager-view-item.html

-        {{^metadata.description}}
-            <p class="muted"><em>{{Strings.EXTENSION_NO_DESCRIPTION}}</em></p>
-        {{/metadata.description}}
+        <span class="ext-full-description">


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

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

TomMalbran · 2014-07-16T19:23:20Z

@le717 Sorry for not getting back to you earlier. This is looking good, I think that we can do those few changes and then merge it. You also need to merge with master.

le717 · 2014-07-16T20:20:44Z

@TomMalbran No problem. I understand not being able to do this stuff all the time. :)

Changes for all but the test pushed. I will fix that ASAP, can't code anymore today.

So, my updated TODO for the code review now looks like:

Fix test
Any other review changes
Rebase
Merge with master

marcelgerber · 2014-07-21T13:17:35Z

src/utils/StringUtils.js

+            if (lastSpaceChar < len && lastSpaceChar > -1) {
+                str = str.substr(0, lastSpaceChar);
+            }
+        return str;


Wrong indentation

le717 · 2014-07-21T16:28:17Z

@TomMalbran Changes pushed, ready for rereview. The original description unit test is broken for some reason, not sure how to to fix it quite yet.

EDIT And for some reason Travis does not build my PR at all anymore...

le717 · 2014-07-21T22:32:55Z

@TomMalbran Unit tests fixed, ready for review. I'm doing my best to get this in for Release 42 so the extension manager doesn't so odd now that #7995 is merged. :)

TomMalbran · 2014-07-21T23:01:08Z

src/extensibility/ExtensionManagerView.js

-
+
+        if (info.metadata.description !== undefined) {
+            context.hasShortDescription = StringUtils.truncate(info.metadata.description, 140);


It doesn't look like we need hasShortDescription. We could just have info.metadata.shortdescription = StringUtils.truncate(info.metadata.description, 140);

I did this so an info.metadata.shortdescription key would not be created for every extension (like you wanted). Without it, most of the info.metadata.shortdescription keys will have a value of undefined.

Yes, but right now it works different from before. So it can be changed again. Before shortDesc and desc where the same for most, which meant duplicated strings. But now it is undefined, so that is fine.

Fixed and pushed.

TomMalbran · 2014-07-21T23:01:56Z

You still need to fix the merge issue. You can merge with master and then fix it.

…ords (#8216)

TomMalbran · 2014-07-22T00:54:42Z

src/thirdparty/CodeMirror2

@@ -1 +1 @@
-Subproject commit 6c0cd2b56b50837a010bd27f322a57edfbe9fee9
+Subproject commit 64bee5830b6f838c2c9be6dded98a5dac794dbcd


This shouldn't be there. You need to update the submodules

Yikes. I was watching for that but it seems I missed it.

TomMalbran · 2014-07-22T01:01:50Z

I think this is almost ready besides those last comments. But I noticed that most short descriptions are just a few chars shorter. Maybe we could allow more chars before using a short description.

TomMalbran · 2014-07-22T01:03:21Z

We are about to branch to release, so I am not sure if it can fit into release 41. We have to be sure that is bug-free and that it does not causes any other bug if we want to merge it right now.

le717 · 2014-07-22T01:10:28Z

@TomMalbran How much longer should it be? @larz0 suggested it be the 140 character limit. I've been using this branch as my Brackets version since I started the PR with no issues.

TomMalbran · 2014-07-22T01:17:29Z

@le717 Figuring the best length might be an issue too. I tried with 200 and it feels a bit better. The descriptions are still short enough, and there are less with a short description.

le717 · 2014-07-22T01:30:16Z

@TomMalbran 200 looks good and knocks out mainly the overly long descriptions now. I think @larz0 liked 140 because I mentioned that is the Twitter character limit and devs can (usually) describe their extension within that limit. @larz0 Do you have any further input on this?

dangoor · 2014-07-22T01:33:04Z

I personally find 140 characters to be painfully limited. 200 seems reasonable.

TomMalbran · 2014-07-22T01:36:20Z

I see. 140 is a good limit, but since is a recent thing, I see several extensions that extend just a little bit more than that, so 200 seems to fit better with us at this point.

le717 · 2014-07-22T01:39:38Z

@TomMalbran @dangoor Limit increased to 200 characters and change pushed.

Truncate extension descriptions

TomMalbran · 2014-07-22T02:13:10Z

Thanks. Merged :)

le717 · 2014-07-22T02:18:25Z

I have such a huge smile on my face right now I probably look silly. :D

TomMalbran · 2014-07-22T02:59:03Z

Hehe :)

larz0 · 2014-07-22T03:10:21Z

le717 mentioned this pull request Jun 29, 2014

Extensions Description need truncated #8216

Closed

le717 reviewed Jun 30, 2014
View reviewed changes

marcelgerber reviewed Jul 1, 2014
View reviewed changes

TomMalbran reviewed Jul 1, 2014
View reviewed changes

le717 reviewed Jul 7, 2014
View reviewed changes

marcelgerber reviewed Jul 21, 2014
View reviewed changes

TomMalbran reviewed Jul 21, 2014
View reviewed changes

Triangle717 added 2 commits July 21, 2014 19:07

Shorten extension descriptions to 140 characters, do not display keyw…

038a60f

…ords (#8216)

Merge remote-tracking branch 'upstream/master' into issue-8216

e1dc1bb

TomMalbran reviewed Jul 22, 2014
View reviewed changes

Changes per review

bbe17ff

Increase limit to 200 characters

2bd4f7b

TomMalbran added a commit that referenced this pull request Jul 22, 2014

Merge pull request #8282 from le717/issue-8216

8ce9221

Truncate extension descriptions

TomMalbran merged commit 8ce9221 into adobe:master Jul 22, 2014

le717 deleted the issue-8216 branch October 21, 2014 19:47

revi mentioned this pull request May 25, 2015

Korean translation update #11101

Merged

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Truncate extension descriptions brackets-archive/bracketsIssues#7405

Open

3 tasks



		if (info.metadata.description !== undefined) {
		context.hasShortDescription = StringUtils.truncate(info.metadata.description, 140);

		@@ -1 +1 @@
		Subproject commit 6c0cd2b56b50837a010bd27f322a57edfbe9fee9
		Subproject commit 64bee5830b6f838c2c9be6dded98a5dac794dbcd

Truncate extension descriptions #8282

Truncate extension descriptions #8282

Conversation

le717 commented Jun 29, 2014

le717 commented Jun 30, 2014

le717 commented Jun 30, 2014

Choose a reason for hiding this comment

larz0 commented Jun 30, 2014

Choose a reason for hiding this comment

le717 commented Jul 1, 2014

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

le717 commented Jul 1, 2014

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

le717 commented Jul 1, 2014

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

le717 commented Jul 2, 2014

TomMalbran commented Jul 2, 2014

le717 commented Jul 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Jul 16, 2014

le717 commented Jul 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le717 commented Jul 21, 2014

le717 commented Jul 21, 2014

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

TomMalbran commented Jul 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Jul 22, 2014

TomMalbran commented Jul 22, 2014

le717 commented Jul 22, 2014

TomMalbran commented Jul 22, 2014

le717 commented Jul 22, 2014

dangoor commented Jul 22, 2014

TomMalbran commented Jul 22, 2014

le717 commented Jul 22, 2014

TomMalbran commented Jul 22, 2014

le717 commented Jul 22, 2014

TomMalbran commented Jul 22, 2014

larz0 commented Jul 22, 2014