codetour icon indicating copy to clipboard operation
codetour copied to clipboard

feat: add default title for new step

Open daiyam opened this issue 2 years ago • 5 comments

This PR allows to customize the default title when creating a new step.

By default, it will create the title with the format {{file}}.

The available variables are:

  • file
  • line
  • text

daiyam avatar Jun 06 '22 10:06 daiyam

I like the idea of a default title, for steps that don't have an explicit title. But I'm not sure if it makes sense to actually set the title in the tour file, as opposed to dynamically deriving the title at render-time.

The reason I say that is because the ".tour" file could be unnecessarily duplicating the file or text contents, and also, if the user wanted to hand edit the tour file (which isn't uncommon), they'd now have to edit it twice.

So I'd recommend moving the title formatting logic to the step tree node, and using it purely as a display-time behavior. If you check out the getStepLabel function in utils.ts, you can see there's existing logic for deriving dynamic step labels. So we need to interop with that as well.

Also, I'm not sure if this property needs to allow arbitrary text? As opposed to representing an enum? I think file/line/description make sense, but I'm not sure eod a compelling use case to set the title to a static string? And making it an enum would provide the end-user with auto-complete when editing their settings.

lostintangent avatar Jun 11 '22 14:06 lostintangent

Yep, I think you are right for the text variable. It should read the content only at display-time. I will change that.

daiyam avatar Jun 11 '22 15:06 daiyam

I think we should do the same for file and line as well. For example, if I move the step to a new line, then the title content would be stale if I was using a line based title. And so we'd either need to update the title when the step is edited, or just make the title property dynamic. I feel like the layer is a cleaner solution, and keeps the tour file free us content that can be easily derived.

Also, by making this a dynamic behavior, it allows the end-user to control it, as opposed to the tour author. And if I want to see the text contents for steps, I should be able to control that, regardless what preference the tour author had.

lostintangent avatar Jun 11 '22 16:06 lostintangent

@lostintangent I've moved the rendering of the title at display-time.

So if a step has "title": "{{file}} - {{line}}", it will be displayed as src/extension.ts - 89 (based on the data of the step). {{text}} is also supported and the file is only read when needed.

daiyam avatar Jun 12 '22 10:06 daiyam

After thinking about this change a bit more, I'm not sure if it adds enough value for the complexity that it introduces. CodeTour already has a ton of customizeability, and so I want to be really thoughtful about new settings, and making sure they're worth the concept count.

In practice, great tours set explicit step titles, and so then it's just a question about what the step title should be if one isn't explicitly set. The current behavior is actually meant to encourage authors to set an explicit title, and so I'm not sure if I actually want this setting, since it might encourage the "wrong" behavior.

Also, this PR is effectively introducing two distinct things: default step titles, and allowing dynamic placeholders in titles. I think the later could be interesting, but I think I'd prefer to hear more feedback about it, before introducing it.

I really appreciate this contribution, but I think I'd rather create a design issue and discuss this behavior, and see if we can solicit some feedback before adding it into the extension.

lostintangent avatar Jun 19 '22 16:06 lostintangent