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

feat: add default title for new step #243

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

daiyam
Copy link
Contributor

@daiyam daiyam commented Jun 6, 2022

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

@lostintangent
Copy link
Member

lostintangent commented Jun 11, 2022

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.

@daiyam
Copy link
Contributor Author

daiyam commented Jun 11, 2022

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

@lostintangent
Copy link
Member

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.

@daiyam
Copy link
Contributor Author

daiyam commented Jun 12, 2022

@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.

@lostintangent
Copy link
Member

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.

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