[WIP] Ability to insert code snippets into files (addresses Issue#1630) #1826

twigz20 · 2017-03-07T21:30:50Z

This is still a work in progress.

I've currently got the core functionality in place. However, I feel as though it lacks in terms of efficiency.

As you can see in the gif above there is a slight freeze in the UI after inserting a snippet. For the time being, I'm storing all the snippets in snippets.js. On initial load up of Thimble, I've set the default file type to be HTML because there is no way to determine what file I'm on at that point in time.
Afterward, whenever the file is changed Thimble receives an activeEditorChanged event which also comes with the full file name. With access to that information, I'm then able to identify the current file type and repopulate the li elements of the snippets menu.

Any further assistance would be highly appreciated.

flukeout · 2017-03-07T23:29:12Z

Hey @twigz20 - really excited to see this. I'll try out your branch and get back to you with some design guidance sometime tomorrow. Thanks for your patience!

flukeout · 2017-03-09T17:47:20Z

Sorry for the delay @twigz20 - will tackle this today.

flukeout · 2017-03-09T17:52:31Z

@twigz20 Would you mind rebasing this to the latest master so it merges cleanly?

flukeout · 2017-03-09T19:09:09Z

Started a couple of issues related to this...

Design of the Menu - #1826
Ideas for Snippet Types - https://github.com/mozilla/thimble.mozilla.org/issues/1857

Pomax · 2017-03-11T01:05:46Z

This PR looks like it needs a rebase to make sure it stays in sync with master so it can be merged when it's ready

humphd · 2017-03-16T20:14:31Z

@twigz20 how is this going? Looks like we've stalled a bit.

flukeout · 2017-03-29T22:48:32Z

Hey @twigz20 - what's the state of this PR? Let me know how I can help - is it ready for testing?

twigz20 · 2017-03-29T23:43:43Z

@flukeout Yes it is. Although i'd like to try using the design mockup you showed here: https://github.com/mozilla/thimble.mozilla.org/issues/1855

flukeout · 2017-03-30T17:03:53Z

Awesome @twigz20 - looks like you got the 'detect file' functionality in there too! This is exciting. So how do you want to proceed? I'm happy to take over for a bit and add some styling / markup for the updated menu in #1855 or is that something you'd like to tackle?

Let me know. I think we should go with this option...

This allows people to copy & paste the code as well. While I like the 'flyout' preview - I think losing copy paste would be annoying.

What do you think? In the meantime, I'll provide an icon for the snippet button shortly.

flukeout · 2017-03-30T17:17:27Z

Hey @twigz20 - just checking into the markup and noticed you're using tabs instead of spaces...

Please switch to spaces for consistency. Cheers!

flukeout · 2017-03-30T17:41:49Z

OK, I made an icon - snippets-icon.zip

Here's how it looks...

Here's how to make it look like that...

<div title="Snippets" id="editor-pane-nav-snippets">
  <img src="/img/icon/snippets.svg">
</div>

#editor-pane-nav-snippets {
  background-color: #2B2B2B;
  height: 30px;
  width: 30px;
  text-align: center;
  border-radius: 50%;
  margin-left: 6px;
  position: relative;
  .icon;

  img {
    opacity: 0.5;
    height: 20px;
    width: 20px;
    position: absolute;
    top: 5px;
    left: 5px;
  }
  
  &:hover img {
    opacity: .7;
  }
}

twigz20 · 2017-03-30T20:21:48Z

@flukeout That would be very helpful if you were to take over a bit. Will make the appropriate changes!

I'm working on trying to improve the efficiency of how I'm loading the snippets and creating the menus. I'm also trying to figure out a way to get the Bramble to send an onActiveEditorChanged event so when the project loads it can default to the appropriate snippets list.

twigz20 · 2017-03-31T16:17:47Z

@gideonthomas to check in with @flukeout about markup for the dropdown

flukeout · 2017-03-31T16:41:14Z

@twigz20 I'm going to work on the menu on top of the current state of your branch and submit a PR against your working branch - sound good?

I'll also include the icon for the dropdown menu.

twigz20 · 2017-03-31T17:50:31Z

@flukeout Yes, thats sounds good!

flukeout · 2017-03-31T19:33:33Z

Hey @twigz20 - I made a PR against your working branch with the markup for the menu. It's got the icon and localized strings too.

Snippet menu markup twigz20/thimble.mozilla.org#1

Should look like this...

I'll let you sort out the functionality along with your other changes. The idea is that the snippet type will be preselected in the header depending on the type of file you have. HTML will be the default.

The user should be able to toggle between the snippet types by clicking those options too.

Let me know if you have any questions!

twigz20 · 2017-03-31T19:40:51Z

@flukeout Thanks! I'll begin testing will let you know if I anything comes up! I do have 1 question. In nav-options.html , in the div i noticed there is a data-adapt-order="4" field. Care to explain to me why is it used? Since I haven't added that to mine.

flukeout · 2017-03-31T19:46:36Z

Yeah, the data-adapt-order attribute is something I created to handle cases when the editor bar is too narrow to fit all of the icons. Items that have that attribute basically start disappearing (or being modified in some way) according to their order. So adapt-data-order=1 is the first item to be removed, and also means it's the least important. It's a new feature, but you should be able to observe it action when you change the width of the editor and previews.

flukeout · 2017-03-31T19:47:28Z

So anyway, if you are able to get the different snippet types in there and update your PR, then I can take another design pass.

gideonthomas · 2017-04-03T17:59:38Z

@twigz20 let me know how far you get with implementing this with the approach I mentioned in class.

twigz20 · 2017-04-04T21:23:34Z

@flukeout Alright, I've gotten a "dirty" implementation of it working with the new design markup. See below.

@gideonthomas I've been attempting the method you suggested. However, as soon as i add in this 1 line var snippets = require("../../lib/snippets"); into editor.js thimble would no longer be connected and instead displays this to me. Note: I've placed snippets.js into the appropriate directory. I've even tried making `snippets.js' an empty file.

So I'm not able to progress any further. Any ideas how I can solve this? @humphd in case you know about this as well!

humphd · 2017-04-05T18:28:08Z

@twigz20 what's on your dev console when you load it? That error doesn't look like a require error but rather a network error.

twigz20 · 2017-04-05T20:59:39Z

@humphd On first refresh my dev console displays this:

Then with each refresh attempt, the dev console is blank

It doesn't give me any information.

humphd · 2017-04-05T21:04:35Z

I'd say your server isn't running. Better debug that, run npm run logs and see what's happening.

twigz20 · 2017-04-05T21:36:49Z

@humphd After running npm run logs i've been getting this error:

humphd · 2017-04-05T22:10:07Z

define is not defined is my favorite error message of the day! So it looks like you're doing this in node.js code, which is different from the requirejs stuff we do in the browser. Do you need to load this module in the browser and in node on the server, or just the server? Basically, you need to rewrite snippets.js to be a proper Common JS module for node.

twigz20 · 2017-04-21T18:50:23Z

@gideonthomas Done. Let me know if i missed anything.

gideonthomas · 2017-04-21T19:15:10Z

@twigz20 awesome! Thanks! I'll open up a PR against your branch with a couple of changes. Thanks for getting all of this done!

gideonthomas · 2017-04-22T16:00:45Z

@twigz20 opened up a PR against your branch!

Rely on templating to generate snippet element state

gideonthomas

@twigz20 awesome work! make these final changes and let's get @humphd to do a quick final review of this.

gideonthomas · 2017-04-25T23:18:14Z

public/editor/scripts/editor/js/fc/bramble-menus.js

@@ -3,6 +3,7 @@ define(function(require) {
  var $ = require("jquery");
  var PopupMenu = require("fc/bramble-popupmenu");
  var analytics = require("analytics");
+  var fileType = "HTML";


you don't use this anymore so let's get rid of it.

gideonthomas · 2017-04-25T23:21:45Z

server/lib/snippets.js

+  }
+}];
+
+const CSS = [{


darn CSS is an object on window. @twigz20 I would advise that you make all of these lowercase throughout your code.

gideonthomas · 2017-04-25T23:25:00Z

locales/en-US/messages.properties

+# Code Snippet Data
+snippetHTMLComment=Comment
+snippetHTMLCommentTitle=Add a Comment
+snippetHTMLCommentData=<!-- Your HTML comment here -->\n


@TheoChevalier I would love your input here. I'm not sure if this is a good way to localize code snippets that we can insert into Thimble's editor. I wanted to localize code snippets that have text content in them. However, each string has a bunch of \n's in them as well as spaces before tag names for formatting purposes when they are rendered. I don't think I was able to come up with a way around this. Let me know what you think.

Thanks for asking, @gideonthomas :) That’s indeed tricky, and I’m not sure there’s a perfect solution. Having a loooong string with lots of \n and embedded code would work, technically, but I think what would be better here is extracting the text from the code snippets, then doing the formatting on the code side.

To be more explicit:
snippetHTMLCommentData=Your HTML comment here

In .js file, somewhere:
var myString = '\n';

Concatenating strings is generally a bad practice, but in this case it’s actually fine since we don’t want localizers to change the markup/code. Would that work with the rest of the snippets? It can be okay to include pieces of code when it makes string extraction easier, as long as there is a comment saying that this part should not be translated.

In the example above, we would add a comment explaining the context (concatenated with )

Does that make sense, or maybe I’m missing something?

no makes perfect sense! Thanks for the input! I think I'll go with that approach.

twigz20 · 2017-04-28T18:42:17Z

@gideonthomas Changes Made!

gideonthomas · 2017-04-29T00:54:59Z

@twigz20 awesome! Thanks, expect another PR (from me) to your branch soon which will incorporate the localization changes suggested above.

flukeout · 2017-05-02T21:29:14Z

Just an update to keep you in the loop @twigz20: @gideonthomas has created a branch with localization changes and we're working on it together to finish it up. Expect a PR against your branch within a day or so! Thanks for your patience - we are close!

Code snippets

More l10n fixes

TheoChevalier · 2017-05-04T19:09:16Z

locales/en-US/messages.properties

+snippetJSIfElseData=if (condition1) {\n   // instructions for condition 1\n} else if (condition2) {\n   // instructions for condition 2\n} else {\n   // fallback instructions\n}\n
+snippetJSComment=Comment
+snippetJSCommentTitle=Add a Comment
+snippetJSCommentData=/* Your Javascript comment here */\n


TheoChevalier

Thanks for the fixes @gideonthomas :) Had a quick look at the l10n part, everything looks good to me 👍

gideonthomas · 2017-05-04T19:45:51Z

Thanks for the review :)

gideonthomas · 2017-05-04T20:09:39Z

Thanks a lot for the hard work @twigz20! You've added an amazing feature to Thimble :)

twigz20 · 2017-05-04T20:27:20Z

@gideonthomas Thanks! I couldn't have done it without the support of the core team!

flukeout · 2017-05-04T22:25:52Z

Yeah awesome work @twigz20 - really happy we got this in! I changed a lot of the snippets, give them a try to see if they make sense or need some changes! 👍 💯

flukeout added the PR – Changes Requested label Mar 16, 2017

twigz20 force-pushed the Bug-1630-Adding-Code-Snippets-Thimble branch from eb28ded to 1a83dd0 Compare March 27, 2017 19:40

gideonthomas self-assigned this Mar 30, 2017

Pomax requested a review from gideonthomas March 30, 2017 20:32

Modified to allow better handling of snippets

08cb0bb

twigz20 force-pushed the Bug-1630-Adding-Code-Snippets-Thimble branch from 6287102 to 08cb0bb Compare April 21, 2017 18:45

Added Missing Semicolon

60882a8

Rely on templating to generate snippet element state

24e4fb8

Gideon Thomas and others added 3 commits April 22, 2017 14:14

Localize the snippets

c4e7217

Fix styling on FF

c829c2c

Merge pull request #2 from gideonthomas/code-snippets

e511cb4

Rely on templating to generate snippet element state

gideonthomas suggested changes Apr 25, 2017

View reviewed changes

Changed Snippet Types to lowercase

346b8b4

Update localization strategy

16f3275

Gideon Thomas and others added 6 commits May 2, 2017 17:32

Derp

4412328

Updated code snippet feature (#3)

2f1c4aa

L10n fixes

a166b99

Merge pull request #3 from gideonthomas/code-snippets

8d31ee6

Code snippets

More l10n fixes

091053b

Merge pull request #4 from gideonthomas/code-snippets

8bf1fc6

More l10n fixes

gideonthomas approved these changes May 4, 2017

View reviewed changes

TheoChevalier reviewed May 4, 2017

View reviewed changes

TheoChevalier approved these changes May 4, 2017

View reviewed changes

gideonthomas merged commit aa63cc8 into mozilla:master May 4, 2017

[WIP] Ability to insert code snippets into files (addresses Issue#1630) #1826

[WIP] Ability to insert code snippets into files (addresses Issue#1630) #1826

Conversation

twigz20 commented Mar 7, 2017

flukeout commented Mar 7, 2017

flukeout commented Mar 9, 2017

flukeout commented Mar 9, 2017

flukeout commented Mar 9, 2017 • edited

Pomax commented Mar 11, 2017

humphd commented Mar 16, 2017

flukeout commented Mar 29, 2017

twigz20 commented Mar 29, 2017

flukeout commented Mar 30, 2017 • edited

flukeout commented Mar 30, 2017

flukeout commented Mar 30, 2017 • edited

twigz20 commented Mar 30, 2017

twigz20 commented Mar 31, 2017

flukeout commented Mar 31, 2017 • edited

twigz20 commented Mar 31, 2017

flukeout commented Mar 31, 2017

twigz20 commented Mar 31, 2017 • edited

flukeout commented Mar 31, 2017

flukeout commented Mar 31, 2017

gideonthomas commented Apr 3, 2017

twigz20 commented Apr 4, 2017 • edited

humphd commented Apr 5, 2017

twigz20 commented Apr 5, 2017 • edited

humphd commented Apr 5, 2017

twigz20 commented Apr 5, 2017 • edited

humphd commented Apr 5, 2017

twigz20 commented Apr 21, 2017

gideonthomas commented Apr 21, 2017

gideonthomas commented Apr 22, 2017

gideonthomas left a comment

Choose a reason for hiding this comment

gideonthomas Apr 25, 2017

Choose a reason for hiding this comment

gideonthomas Apr 25, 2017

Choose a reason for hiding this comment

gideonthomas Apr 25, 2017

Choose a reason for hiding this comment

TheoChevalier Apr 28, 2017

Choose a reason for hiding this comment

gideonthomas Apr 29, 2017

Choose a reason for hiding this comment

twigz20 commented Apr 28, 2017

gideonthomas commented Apr 29, 2017

flukeout commented May 2, 2017

TheoChevalier May 4, 2017

Choose a reason for hiding this comment

TheoChevalier left a comment

Choose a reason for hiding this comment

gideonthomas commented May 4, 2017

gideonthomas commented May 4, 2017

twigz20 commented May 4, 2017

flukeout commented May 4, 2017

flukeout commented Mar 9, 2017 •

edited

flukeout commented Mar 30, 2017 •

edited

flukeout commented Mar 30, 2017 •

edited

flukeout commented Mar 31, 2017 •

edited

twigz20 commented Mar 31, 2017 •

edited

twigz20 commented Apr 4, 2017 •

edited

twigz20 commented Apr 5, 2017 •

edited

twigz20 commented Apr 5, 2017 •

edited