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.

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

Added HtmlSpecialCharsCodeHints as default extension #3237

Merged
merged 10 commits into from Mar 27, 2013

Conversation

@WebsiteDeveloper
Copy link
Contributor

commented Mar 24, 2013

I added CodeCompletition for HtmlSpecialChars as requested in https://trello.com/card/code-completion-for-special-characters/4f90a6d98f77505d7940ce88/826

@ghost ghost assigned redmunds Mar 26, 2013
@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2013

This looks great. I have a few suggestions:

  • It would be nice if this worked for other languages besides HTML like PHP and JavaScript and others, even in CSS it might be useful for the pseudo elements.
  • You might want to use a case insensitive compare function in the sort.
@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2013

@TomMalbran Character entity references are case-sensitive. For example, &dagger and &Dagger are different characters.

@@ -0,0 +1,104 @@
[
"&lsquo", "&rsquo", "&sbquo", "&ldquo", "&rdquo", "&bdquo",

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 26, 2013

Contributor

I think it will be easier to manage this list if it's a single column sorted alphabetically

* @constructor
*/
function SpecialCharHints() {
this.primaryTriggerKeys = "&ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#0123456789";

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 26, 2013

Contributor

I think "&" is the only trigger key that you need here, since that's the first char of every entity.

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Mar 26, 2013

Author Contributor

those other keys have to be in there because otherwise the hinting sesssion would stop after typing another character after the ampersand.

@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2013

I know, what I mean is that it would be better if for example á = á would comes after Á = Á. Right now it shows all the capital letter special chars first and then all the lower letter special chars. The sort could compare first in lower case, and then in upper case for equal cases.

[Randy] Sorry, I read you comment wrong.

// Register code hint providers
var specialCharHints = new SpecialCharHints();

CodeHintManager.registerHintProvider(specialCharHints, ["html"], 0);

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 26, 2013

Contributor

I'm seeing a weird problem in this case:

  1. Start with a paragraph: <p></p>
  2. Place cursor between the open and closing tags and type "&"
  3. The entity hint list is displayed as expected
  4. Press Esc to dismiss list
  5. Press Ctrl-space to re-invoke the hints list

Results:
Hints list is displayed, but it's a list of HTML Tags!

It might have something to do with using a priority of 0 to register these hints, which is the default priority. On the other hand, there should be no conflicts with the HTML hints, so not really sure.

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Mar 26, 2013

Author Contributor

actually it the problem is, that the HTML tags hint provider can also provide hints for those and gets called before the specialCharHints, but when i changed the priority to one plus a few other tweaks in the has hints function it worked.
So fixed.

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

That problem is fixed, but I'm seeing another problem I think it related. Code hints work correctly inside of a tag, but if I type a "raw" & I don't get the hints. For example:

<body>
    &
</body>
@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2013

I'm seeing the entity hints displayed if I type an ampersand inside an attribute value string. For example: <a href="results.html?id=3&lang=en">click me</a>.

this.currentQuery = query = this._getQuery();
result = $.map(specialChars, function (value, index) {
if (value.indexOf(query) === 0) {
return value + " <span style=\"float: right;\">" + value + ";</span>";

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 26, 2013

Contributor

Create a class and apply class to span tag, instead of inline css.

Also, the code looks cleaner if you nest single-quotes inside of double-quotes:

    return value + " <span class='entity-display-char'>" + value + ";</span>";

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Mar 26, 2013

Author Contributor

fixed.
should i add the style to the brackets.less file?

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Mar 26, 2013

Contributor

No, you should create a CSS file in your extension directory and load it with ExtensionUtils.loadStyleSheet(module, "styles.css"); on AppInit.htmlReady.

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Mar 26, 2013

Author Contributor

i added that.

"&Omega", "&omega",

"&#33",
"&#35", "&#36", "&#37",

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 26, 2013

Contributor

There's a problem filtering numeric entities:

  1. Type: &#
  2. List of numeric entities is shown as expected
  3. Type: 3

Results:
Left column of list now shows only: 3, 5, 6, 7, 9

Expected:
Complete numeric entities to be displayed: &#33, &#35, &#36, &#37, &#39

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Mar 26, 2013

Author Contributor

fixed.

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2013

Done with initial review.

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2013

I found some more entities listed here: http://www.w3.org/TR/html4/sgml/entities.html

@TomMalbran

This comment has been minimized.

You could use return a.localeCompare(b). I am not sure if the > works as expected for strings. And you should remove the log from the next line.

This comment has been minimized.

Copy link
Owner Author

replied Mar 26, 2013

yeah i will remove that in the next commit.

@WebsiteDeveloper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2013

@redmunds i adressed the issues you remarked.
So ready for re-review

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

The trailing ; char should be displayed in list so user sees what they will get. For example, display &nbsp; instead of &nbsp .

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

The numerical entities should be ordered by numerical value, not string value. For example, I see (skipping some values fro brevity):

&#125;
&#33;
&#8226;
&#91;

But it should be:

&#33;
&#91;
&#125;
&#8226;
@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

Thanks for adding he other entities I found. There are typos: &asyp; should be &asymp;, and &exists; should be &exist;

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

I think the name of this extension is too long. Please rename to: HtmlEntityCodeHints

return query !== null;
} else {
return implicitChar === "&" || query !== null;
}

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

You don't need else, and removing it makes it easier to see that something is returned in all cases:

        if (implicitChar === null) {
            return query !== null;
        }

        return implicitChar === "&" || query !== null;
this.currentQuery = query = this._getQuery();
result = $.map(specialChars, function (value, index) {
if (value.indexOf(query) === 0) {
var shownValue = (value.indexOf("#") === -1) ? value.replace("&", "&amp;") : value.replace("#", "&#35;");

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

(query.indexOf("#") === -1) ? query.replace("&", "&amp;") : query.replace("#", "&#35;") should be moved to a function since it is used agan below.

return query;
} else {
return null;
}

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

Remove else:

        if (startChar !== -1 && HTMLUtils.getTagAttributes(this.editor, this.editor.getCursorPos()).length === 0) {
            return query;
        }

        return null;
* Inserts a given HtmlSpecialChar hint into the current editor context.
*
* @param {String} hint
* The hint to be inserted into the editor context.

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

Parameter name is completion.

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

Done with second code review.

@WebsiteDeveloper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2013

@redmunds all issues adressed, ready for another review.

"&ang",
"&aring",
"&Aring",
"&asymp",

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

I guess you didn't see the edit to my previous comment, but &asymp; is missing (was a typo).

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Mar 27, 2013

Author Contributor

@redmunds already readded both

"&eth",
"&ETH",
"&euml",
"&Euml",

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

I guess you didn't see the edit to my previous comment, but &exist; is missing (was a typo).

}

return a.localeCompare(b);
});

This comment has been minimized.

Copy link
@redmunds

redmunds Mar 27, 2013

Contributor

Move this code to a named function that you can pass to the sort() method.

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

This looks great. One last comment!

@WebsiteDeveloper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2013

@redmunds calling for another review (hopefully the last)

@redmunds

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2013

Looks good. Merging.

redmunds added a commit that referenced this pull request Mar 27, 2013
Added HtmlSpecialCharsCodeHints as default extension
@redmunds redmunds merged commit cb738dd into adobe:master Mar 27, 2013
1 check passed
1 check passed
default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.