Adds code folding support for handlebars template files by thehogfather · Pull Request #12675 · adobe/brackets

thehogfather · 2016-08-14T21:30:10Z

Adds a new range finder to detect foldable regions and enable code folding for blocks and helpers in handlebars template files.

…_range_finders

ficristo · 2016-08-21T08:49:55Z

+ * @author Patrick Oladimeji
+ * @license MIT
+ * @date 14/08/2016 22:04:21
+ */


I think you have to use the standard license comment of adobe on newer file now the code folding extension is in the core.

no problem - I'll remove it -artefact of automatic templates

ficristo · 2016-08-21T08:59:39Z

I don't know handlebar, so I left some general comments.
If nobody will look at the correctness of this, I'll give a try on some examples and eventually merge.

thehogfather · 2016-08-21T18:48:03Z

-
+        CodeMirror.registerHelper("fold", "handlebars", handlebarsFold);
+        CodeMirror.registerHelper("fold", "htmlhandlebars", handlebarsFold);
+        


will remove space.

ficristo · 2016-08-22T08:42:19Z

Another nit but in general Brackets use double quote for strings.

ficristo · 2016-08-23T18:14:12Z

@@ -0,0 +1,174 @@
+/**


I think you have to include the full licence comment here (like in other source files).
Beware the date has to be 2016 - present.

/* * Copyright (c) 2016 - present Adobe Systems Incorporated. All rights reserved. * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), *.... *...

thehogfather · 2016-08-27T21:41:29Z

+ * @author Patrick Oladimeji
+ * @date 14/08/2016 22:04:21
+ */
+


@ficristo BTW I notice this empty line is being introduced throughout the code base - although it conflicts with an existing recommendation in the fairly stale JS Docs guidelines. I am guessing that the guideline has simply not been updated?

AFAIK the guidelines are still true. But until we can have stricter rules on our linter sometimes we miss things.

ficristo · 2016-09-28T18:34:51Z

-        CodeMirror.registerHelper("fold", "django", CodeMirror.helpers.fold.brace);
-        CodeMirror.registerHelper("fold", "tornado", CodeMirror.helpers.fold.brace);
+        CodeMirror.registerHelper("fold", "handlebars", handlebarsFold);
+        CodeMirror.registerHelper("fold", "htmlhandlebars", handlebarsFold);


@thehogfather is the second parameter a CodeMirror mode? Where do you use it?
It seems to me that Mustache is not part of your PR, but handlebars is a Mustache like template.
And we use Mustache in the codebase.
So if it that param is a mode, I was wondering if we can use text/x-brackets-html or htmlmixed

yup that parameter is a CodeMirror mode. The name of the mode is used internally by CodeMirror to create a lookup table for the fold helper for a given mode.

ficristo · 2016-09-29T18:32:47Z

+define(function (require, exports, module) {
+    "use strict";
+    var CodeMirror = brackets.getModule("thirdparty/CodeMirror/lib/codemirror"),
+        endOfLineSpaceRegex = /\s$/,


My little mind cannot understand this: are you looking for a single space, right? For which cases?

This looks for a space that occurs at the end of a string - and I have used it to scan a line until a space is reached. Very useful in finding potential opening range tags - e.g., {{ and {{foo in the following scenario:

{{ foo bar=baz var-1=goo }}

or

{{foo bar=baz var-1=goo }}

And based on what is currently on the stack of the opening tags we try to search for a closing tag. I will add better comments to these. Perhaps endOfLineSpaceRegex is actually not a good name and naming the variable might be unnecessary in this scenario as it is only used in one place.

ficristo · 2016-09-29T18:34:13Z

+            to: found.to
+        };
+        openTag = found.string.substring(0, found.string.length - 1);
+        if (openTag[0] === "#" || openTag[0] === "~") {


Here and below I think you have to add ^ and add a test for it and for ~

Good point. I was working with the handlebars page as reference. #* should also be added.

ficristo · 2016-09-29T18:48:57Z

I left a question and a request of change.
I didn't check everything, leaving that to the tests
Could merge master (again)? It will be easier for final check

…ced by ~, ^ or #* updated tests to capture these cases

…_range_finders

thehogfather · 2016-10-02T19:09:58Z

@ficristo PR has been updated.

ficristo · 2016-10-03T06:00:52Z

@thehogfather thank you.
I have some problems running all CodeFolding tests. Can you run them and are all green?

ficristo · 2016-10-03T17:39:57Z

On a PC where I was able to run tests I had a consistent failure on the can be enabled by setting makeSelectionsFoldable' to true` test.

thehogfather · 2016-10-03T19:53:54Z

@ficristo - could you clarify if this use case also fails in use - and does it fail for all three file types, js, html, and handlebars? Could you change the preference in the project settings and see if selection fold is enabled in the editor.
All tests are passing on my mac - and I have no access to a PC to test that out.

ficristo · 2016-10-04T05:25:37Z

Actually I tested it on OSX too. I'll try to understand more the issue.

thehogfather · 2016-10-04T05:28:55Z

And do you get the same failure on OSX?

ficristo · 2016-10-04T05:32:00Z

yes

thehogfather · 2016-10-04T10:30:50Z

Very strange. Let me know if I can help - unfortunately, I have now tried different macs and I still cannot reproduce.

ficristo · 2016-10-04T12:40:54Z

I run again the tests on OSX and there still was a failure. I tryed to add a couple of breakpoints
and the test passed...
Then I tryed to enclose the selectTextInEditor call in a runs function and all the tests passed.

runs(function () {
  selectTextInEditor(start, end);
});

I can only guess there is some async problem, does it make sense to you?

thehogfather · 2016-10-04T19:50:02Z

To be honest I"m a little baffled as to why that would fix it as the selectTextInEditor function itself uses the runs in its definition so the next block should not run until the selection is ready.

ficristo · 2016-10-05T16:41:25Z

I had some problem with code folding tests in the past too.
So, since they work for you, I decided to merge.
Thank you for you patience.

thehogfather added 5 commits August 14, 2016 00:05

added fold range finder for handlebars template files.

3ba8422

update to test handlebars file

50127dc

Added test.hbs file and unit tests for handlebars template files

6de5a09

Merge branch 'master' of https://github.com/adobe/brackets into extra…

82be43c

…_range_finders

added space after module comment

5a363b5

ficristo added the F Code Folding label Aug 16, 2016

ficristo reviewed Aug 21, 2016
View reviewed changes

thehogfather reviewed Aug 21, 2016
View reviewed changes

Addresses code review comments

c04e7f4

ficristo reviewed Aug 23, 2016
View reviewed changes

added license header and stringutils

b40a8db

thehogfather reviewed Aug 27, 2016
View reviewed changes

zaggino added this to the Release 1.9 milestone Aug 28, 2016

ficristo reviewed Sep 28, 2016

View reviewed changes

adds handlebars fold helper to htmlmixed mode

0c48463

ficristo reviewed Sep 29, 2016

View reviewed changes

thehogfather added 2 commits October 2, 2016 20:07

added more handlebars/mustache-related helper tags e.g., tags introdu…

31082ca

…ced by ~, ^ or #* updated tests to capture these cases

Merge branch 'master' of https://github.com/adobe/brackets into extra…

e4da937

…_range_finders

ficristo merged commit 281a4df into adobe:master Oct 5, 2016

ficristo modified the milestones: Release 1.8, Release 1.9 Oct 5, 2016

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

[CLOSED] Adds code folding support for handlebars template files brackets-archive/bracketsIssues#10806

Open


		CodeMirror.registerHelper("fold", "handlebars", handlebarsFold);
		CodeMirror.registerHelper("fold", "htmlhandlebars", handlebarsFold);

Conversation

thehogfather commented Aug 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ficristo commented Aug 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ficristo commented Aug 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ficristo commented Sep 29, 2016

Uh oh!

thehogfather commented Oct 2, 2016

Uh oh!

ficristo commented Oct 3, 2016

Uh oh!

ficristo commented Oct 3, 2016

Uh oh!

thehogfather commented Oct 3, 2016

Uh oh!

ficristo commented Oct 4, 2016

Uh oh!

thehogfather commented Oct 4, 2016

Uh oh!

ficristo commented Oct 4, 2016

Uh oh!

thehogfather commented Oct 4, 2016

Uh oh!

ficristo commented Oct 4, 2016

Uh oh!

thehogfather commented Oct 4, 2016

Uh oh!

ficristo commented Oct 5, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants