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 context selection to debug-panel #58

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

utillity
Copy link

I have created a tree-view panel in DEBUG tab which allows selection of different json files in same or parent directories.

@utillity
Copy link
Author

Hi @johnknoop please let me know if I should change anything or if you will release a new version with this change

@johnknoop
Copy link
Owner

Hi @utillity sorry for the late reply. I've hadn't had any time to look at this yet. Give me a few days and I'll make sure to try it out locally.

@utillity
Copy link
Author

utillity commented May 16, 2023

@johnknoop I am adding a separate PR to also be able to send emails (via sendgrid). Send button will only show up, if respective configuration is provided.

if you want I can also add the changes to this PR instead

@johnknoop
Copy link
Owner

Trying it out right now and I'm seeing some errors.

First of all, I can no longer preview a HBS file. It gives me this error:

image

And when I look at he new section in the Debug panel, it's empty. Hitting "reload" generates this error:

image

@utillity
Copy link
Author

interesting - sounds like there are some files out of sync or not all have compiled correctly. Can you confirm that the refreshTree command is indeed registered inside extension.ts?

@utillity
Copy link
Author

you can compare files with my forked version: https://github.com/utillity/vscode-handlebars-preview

@utillity
Copy link
Author

the first error message suggests that something happened during compilation - can you confirm that compilation succeeded?

@johnknoop
Copy link
Owner

I'll have another look later today or tomorrow. Sorry for not having a lot of time to spend on this. I juggle this extension as a side project.

@johnknoop
Copy link
Owner

Hi @utillity I've finally gotten around to have another look at this. I was able to get it running. However I was thinking... do we really wanna list ALL the context files in the new panel? Maybe it would make more sense to only list the ones that contain the filename of the currently viewed template (excluding .hbs)? Like you suggested in your original issue:

myTemplate.hbs
myTemplate.hbs.json
myTemplate-screnario1.hbs.json
myTemplate-screnario2.hbs.json

In this case, if I was previewing myTemplate.hbs, it would be nice to only see those three json files in the contexts panel, and not all the other ones that are probably "incompatible" with the template.

What's you thinking on that?

@johnknoop
Copy link
Owner

@utillity did you have a chance to reflect on my question above?

@utillity
Copy link
Author

utillity commented Jun 12, 2023

Hi @johnknoop, sorry for the late reply! I realized my structure is a bit different. My directory-structure defines the template name (ie. Access/Declined), while the actual template files within the leaf-directory are for the different notification channels (ie. app.hbs, email.bhs). then I have different scenario .hbs.json files with which I test the template.

image

I understand that this represents a more complex scenario and might not represent what is needed by others. Any thoughts of how I could make this configurable via a setting?

@utillity
Copy link
Author

I could make a Filter files by template filename setting which would cover both scenarios - what do you think?

@utillity
Copy link
Author

if I find a way to add buttons to the preview-page, I could also add support for data-arrays within the .hbs.json file and navigate through them via previous and next buttons in the preview

@johnknoop
Copy link
Owner

A setting like is probably the best solution. Maybe call it Context files panel: Only show files matching active template name or something? If it's turned on, it'll work like I suggested here. If it's turned off, the panel will list all json files in the same folder as the active template.

Then to the question: that do we mean by the "active template". If you have two panes side by side, one with the source code of a template and the other with the preview (which is a common way of working):

image

Then we should of course list the context files that matches "myTemplate.hbs".

But then you open a different template in the left pane, while the right pane still shows the preview of "myTemplate":

image

Which context files should we list then? This requires some thought.

@johnknoop
Copy link
Owner

Another question: whats the pros/cons of putting the new context file switcher in the Debug panel and not in the Explorer panel?

@utillity
Copy link
Author

currently I always use the last template which was navigated to, to show the context files. I ignore the previews so the context-files-list always "moves" with the active window (as long as the active window is a hbs file).

regards "DEBUG" panel. it was just a semantic decision - it's more like debugging to validate the template with different context files and in any case you can put the "preview" section into the secondary panel (no matter, if I put it in the explorer or debug panel). I'm fine with moving it if you prefer the explorer panel.

- Moved section from debug to explorer panel
@utillity
Copy link
Author

I just pushed the changes:

  • moved section from debug to explorer
  • added Context-Filter setting

@utillity
Copy link
Author

oh, I just realized I also merged the sendgrid/email sending changes. It's all in this PR now, sorry about that

Copy link
Owner

@johnknoop johnknoop left a comment

Choose a reason for hiding this comment

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

I've had some time to try this out locally now. Great job! I'll approve this PR if you make a few changes:

  • Rather then destroying and recreating the preview panel when the user selects a different context file, I think we should just update the already open one. That would preserve its position in the tab list, and also not lose the scrollbar position of the preview panel. So basically remove the contextFileName parameter on the PreviewPanelScope constructor, and instead add a new method to the class called switchContextFile(fileUri: string) or something.
  • Remove the sendgrid feature. We can discuss that one in a separate PR, but currently I'm feeling it might be out of scope, since any project that uses Handlebars as a templating engine for e-mails will already have a testing pipeline setup for sending those e-mails.
  • Make the new context files panel list json files compatible with the currently active preview panel rather than the currenctly active editor. I've given it some thought and personally I would find that more intuitive, since it's only once the template has been rendered that you see the results. Please push back on this if you don't agree.
  • Move all the code related to this feature that currently lives in extention.ts into a separate file, since extention.ts is starting to get kinda bloated now.
  • Add docs to the README.md about how to use the settings that govern which files are listed in the panel

src/context-data-tree-provider.ts Outdated Show resolved Hide resolved
src/context-data-tree-provider.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
removed commented-out code
updated readme
@utillity
Copy link
Author

I've had some time to try this out locally now. Great job! I'll approve this PR if you make a few changes:
Thanks!

* Rather then destroying and recreating the preview panel when the user selects a different context file, I think we should just update the already open one. That would preserve its position in the tab list, and also not lose the scrollbar position of the preview panel. So basically remove the `contextFileName` parameter on the `PreviewPanelScope` constructor, and instead add a new method to the class called `switchContextFile(fileUri: string)` or something.

In (your) extensions.ts line 92, openPreviewPanelByDocument(), you dispose the panel and then create a new one. not sure how moving the context-filename to a separate method call changes the behaviour you are describing.

* Remove the sendgrid feature. We can discuss that one in a separate PR, but currently I'm feeling it might be out of scope, since any project that uses Handlebars as a templating engine for e-mails will already have a testing pipeline setup for sending those e-mails.

I agree that a pipeline is in place, but I don't want to deploye the templates each time to test them - this way it's much easier to test template (microsoft outlook has a habit of badly messing with the HTML and distorting email designs). By default (if no API key etc is specified), there is no trace of this feature. I moved all code to a separate file. I hope you accept this feature, as it's of high value to our team to be able to test the email templates without first committing them to the infrastructure.

* Make the new context files panel list json files compatible with the currently active _preview panel_ rather than the currenctly active _editor_. I've given it some thought and personally I would find that more intuitive, since it's only once the template has been rendered that you see the results. Please push back on this if you don't agree.

Hmmm - I tend to disagree because I can use the treeview even before first using your standard "preview" command. If I had no "default" context file there is no reason to first open an errornous preview just to be able to see the tree.

* Move all the code related to this feature that currently lives in `extention.ts` into a separate file, since `extention.ts` is starting to get kinda bloated now.

Done

* Add docs to the README.md about how to use the settings that govern which files are listed in the panel

Done

please consider my suggestions above. I'm happy to discuss also via zoom/teams, if you want.

thanks and cheers, Tilli

@utillity utillity requested a review from johnknoop June 27, 2023 13:04
@johnknoop
Copy link
Owner

In (your) extensions.ts line 92, openPreviewPanelByDocument(), you dispose the panel and then create a new one

This is only done when the user runs the "Open preview" command. It's only a way of preventing duplicate preview panels. We never dispose+recreate the panel when the template or context data changes, which is effectively what happens when you switch context file. The problem with disposing and recreating the panel is that it won't stay in the same place in the tab list, and the scroll position will be reset, which is a bad user experience when switching between context files.

I agree that a pipeline is in place, but I don't want to deploye the templates each time to test them - this way it's much easier to test template (microsoft outlook has a habit of badly messing with the HTML and distorting email designs). By default (if no API key etc is specified), there is no trace of this feature. I moved all code to a separate file. I hope you accept this feature, as it's of high value to our team to be able to test the email templates without first committing them to the infrastructure.

First of all, a lot of people that use this extension are building express sites and other apps that aren't event e-mail related. For them it's just "bloatware" to have a feature called "Send as e-mail using Sendgrid".

For those who use this extension to develop e-mail templates, I suspect there are two distinct feedback cycles going on. The "inner" one which involves this extension. It's when you're actively developing the template. But then you have an outer feedback loop where you actually send the e-mail in the exact same way as you would in production. I for examme have a project that sends transactional e-mails using Mailgun. Merely getting an e-mail in by inbox that was sent using Sendgrid wouldn't be enough of a quality assurance to make my commit my code changes. Because in order to know that it's gonna work in production, I need to make sure that the API call to Mailgun is correct, including the names of HTTP headers etc. So in my case I wouldn't have any use for the Sendgrid feature even in my e-mail project.

In your case, it's really easy to just write a node script that iterates over all the hbs templates and sends an e-mail using Sendgrid. If you name npm script something short like send-emails then it doesn't take more time to run it from the command line than it would to click a menu item in VS Code.

Hmmm - I tend to disagree because I can use the treeview even before first using your standard "preview" command. If I had no "default" context file there is no reason to first open an errornous preview just to be able to see the tree.

Immediately after you switch from one context file to another, where do you look? Nothing changes in the template. It's the preview that changes when you switch context file. Lets compare it to a situation where you're developing an e-commerce website that has an HTML template for the product details page, fed by data from a database. If you play around with the data in your db to test how the rendered page looks with longer or shorter product names, or a different number of product images, then you tend to jump back and forth between your database GUI and the browser, while your code editor isn't really part of it. Isn't this kinda the same thing?

@utillity
Copy link
Author

utillity commented Jul 3, 2023

...The problem with disposing and recreating the panel is that it won't stay in the same place in the tab list, and the scroll position will be reset, which is a bad user experience when switching between context files.

ah, now I got you - will do that

First of all, a lot of people that use this extension are building express sites and other apps that aren't event e-mail related. For them it's just "bloatware" to have a feature called "Send as e-mail using Sendgrid".

The button is not visible if you provide no settings - hence no bloatware feeling to those who don't need it

For those who use this extension to develop e-mail templates, I suspect there are two distinct feedback cycles going on. The "inner" one which involves this extension. It's when you're actively developing the template. But then you have an outer feedback loop where you actually send the e-mail in the exact same way as you would in production. I for examme have a project that sends transactional e-mails using Mailgun. Merely getting an e-mail in by inbox that was sent using Sendgrid wouldn't be enough of a quality assurance to make my commit my code changes. Because in order to know that it's gonna work in production, I need to make sure that the API call to Mailgun is correct, including the names of HTTP headers etc. So in my case I wouldn't have any use for the Sendgrid feature even in my e-mail project.

this feature is for what you would call "inner" level testing. It's just a means for the template developer to ensure different email clients render the mail correctly - it's like the "PREVIEW IN BROWSER" button offered in some editors ;). I don't want to do integration tests - I want to test the template I create - so whether sendgrid or another mail system is being used it doesn't matter.

Unfortunately you don't seem happy with the feature at all - even when it's completely hidden if not configured. I'll just have to keep running my branch in debug mode to be able to test my templates (and my team won't be able to).

In your case, it's really easy to just write a node script that iterates over all the hbs templates and sends an e-mail using Sendgrid. If you name npm script something short like send-emails then it doesn't take more time to run it from the command line than it would to click a menu item in VS Code.

Having to tell the script which HBS and which JSON file to use each time is not very practical. I want to test a specific json file with a template. Especially with outlook email client it's a lot of trial and error.

Immediately after you switch from one context file to another, where do you look? Nothing changes in the template. It's the preview that changes when you switch context file. Lets compare it to a situation where you're developing an e-commerce website that has an HTML template for the product details page, fed by data from a database. If you play around with the data in your db to test how the rendered page looks with longer or shorter product names, or a different number of product images, then you tend to jump back and forth between your database GUI and the browser, while your code editor isn't really part of it. Isn't this kinda the same thing?

I get where you're coming from - so I would track the last preview window, and if none is found, the last editor window.

@johnknoop
Copy link
Owner

johnknoop commented Jul 11, 2023

@utillity

Thanks a lot! Looking forward to merging this.

And I'm not completely opposed to an e-mail sending feature going forward. But in that case I wouldn't wanna tie it solely to SendGrid, but rather try and make it more general. Like being able to send the rendered HTML to any HTTP endpoint, by adding config like this:

"httpTestEndpoints": [
  {
    "name": "Sendgrid",
    "url": "https://api.sendgrid.com/v3/",
    "method": "post",
    "headers": {
      "Authorization": "Bearer <<YOUR_API_KEY>>",
      "Content-Type": "application/json"
    },
    "bodyTemplate": "{'personalizations':[{'to':[{'email':'john.doe@example.com','name':'John Doe'}],'subject':'Hello, World!'}],'content': [{'type': 'text/plain', 'value': '$$RENDERED_OUTPUT$$'}],'from':{'email':'sam.smith@example.com','name':'Sam Smith'},'reply_to':{'email':'sam.smith@example.com','name':'Sam Smith'}}"
  },
  {
    "name": "Mailgun",
    "url": "https://api.sendgrid.com/v3/",
    "method": "post",
    "headers": {
      "Authorization": "Basic <auth>",
      "Content-Type": "multipart/form-data"
    },
    "bodyTemplate": "from=hello@test.com&to=me@example.com&subject=Testing&html=$$RENDERED_OUTPUT$$"
  }
]

Then you could add as many endpoints as you wish, and you wouldn't be limited to e-mail services either for that matter. But this definetely needs to be a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants