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

Issue templates #4278

Merged
merged 16 commits into from Jan 23, 2024
Merged

Issue templates #4278

merged 16 commits into from Jan 23, 2024

Conversation

jgonggrijp
Copy link
Collaborator

I decided to try my hand at GitHub's issue template functionality. The changes can be previewed at my fork, where I temporarily enabled issues. This is probably a better way to review this PR than by reading the YAML files. The top three categories have forms that you can view by clicking "get started".

I had two main goals:

  1. Steer people away from reporting security vulnerabilities through public issue tickets. Those should be communicated privately to me and Jeremy instead.
  2. Guide inexperienced users to document their issues as clearly as possible.

For expert users, there is still the option to skip the forms and create a blank issue.

I may have gone a bit overboard with the forms. I'm open to all suggestions.

@jgonggrijp
Copy link
Collaborator Author

Bump: I would prefer to see opinions from at least two different people (excluding myself), before taking a decision. To anyone considering to do a review: you can skip the code, the preview link in the opening post will show you what it will actually look like in the GitHub web interface.

CC @paulfalgout @Rayraz @GammaGames

@GammaGames
Copy link

I figured out how to preview them, they look good! The bug one might be a little long though, entering all that information would take a while. I left a few comments where I thought they could be combined without missing anything

Copy link
Collaborator

@paulfalgout paulfalgout left a comment

Choose a reason for hiding this comment

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

Fancy

@Rayraz
Copy link

Rayraz commented Aug 9, 2023

The bug form is quite long. I suppose things like logs, research, prior discussions, related tickets, etc. could all be wrapped into simply a remarks field?

Steps to reproduce and example code could possibly be merged into one field as well.

@jgonggrijp
Copy link
Collaborator Author

I figured out how to preview them, they look good! The bug one might be a little long though, entering all that information would take a while. I left a few comments where I thought they could be combined without missing anything

@GammaGames where can I see your comments?

@GammaGames
Copy link

The comments should be in a review, I think? I see it in the feed:
image

@jgonggrijp
Copy link
Collaborator Author

@GammaGames Ah, it says "pending" in that screenshot, which means they are probably only visible to you. GitHub queues your comments and then publishes them all at once when you submit your "overall" review (i.e., the "verdict"). You can make that final step by going to the "Files changed" tab and pressing the green button in the top right.

attributes:
label: Affected engine(s)
description: >
In which browser(s) or other JavaScript environment(s) did you encounter

Choose a reason for hiding this comment

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

Do we have a Backbone.getDebugInfo() function? Would make this step way easier (for both client and server)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we do, but that is an excellent idea. I'll see whether it is easy to add, and otherwise create a ticket for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@GammaGames please have a look at 8fe1bbb while I continue to work on the other review comments. Maybe it is a bit bulky for what it is; I put it all in one commit so it will be easy to revert.

Choose a reason for hiding this comment

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

I think putting it all in one commit makes sense and at a glance it looked good! I'll try to take a closer look and respond to the rest of your comments sometime this weekend

.github/ISSUE_TEMPLATE/Bugs.yml Outdated Show resolved Hide resolved
attributes:
label: Code permalinks
description: >
If possible, provide permalinks to the JavaScript code, configuration,

Choose a reason for hiding this comment

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

I think this is useful, but if we have to cut something this is where we should do it. 99% of users aren't going to do this (especially if they've already got a line and version, that's enough to find the location)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to make sure we are talking about the same thing: did you interpret this question as asking for a permalink to Backbone code, or for a permalink to someone's project code in which the bug manifested?

Choose a reason for hiding this comment

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

I interpreted it as a link to the documentation, I may have misread it. Most repos have a jsfiddle template set up and just ask for a reproducable example

.github/ISSUE_TEMPLATE/Bugs.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/Bugs.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/Bugs.yml Show resolved Hide resolved
id: problem
attributes:
label: Problem statement
description: >-

Choose a reason for hiding this comment

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

I know this is technically something to collect, but it's really just the cause of the problem. They're both asking similar questions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I somewhat disagree here, it's asking for the effects rather than the cause. I know people generally find it more intuitive to discuss the cause before the effect, but it is the effect that actually motivates changes to the documentation. I find it important to start with the "why".

That said, your comment made me realize the naming of the field was a bit vague, so I renamed it to "effect".

Choose a reason for hiding this comment

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

I agree with the name change

.github/ISSUE_TEMPLATE/Features.yml Show resolved Hide resolved
.github/ISSUE_TEMPLATE/Features.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/Bugs.yml Show resolved Hide resolved
@GammaGames
Copy link

@jgonggrijp Thank you, posted! Guess I've accidentally done it correctly the few times I've posted a review

@jgonggrijp
Copy link
Collaborator Author

@GammaGames and @Rayraz, I think I followed up on your review comments (maybe not to the letter but hopefully still in spirit). I merged a bunch of questions, especially in the bug template. Please have another look at the live preview.

The new debugInfo method is up for debate; I kept all changes related to it on separate commits, so they can be easily reverted.

@GammaGames
Copy link

I think the shortened bug report mostly looks good! Is there any way to make the behavior question inputs larger? One of them has their placeholder text truncated:
image

@jgonggrijp
Copy link
Collaborator Author

jgonggrijp commented Aug 18, 2023

I can only choose between single-line <input> and multi-line <textarea>. In the latter case, it is the same size as all other "large" boxes. Which of these options would you prefer?

  • Shorten the placeholder so it fits.
  • Turn the behavior fields into <textarea>s.

By they way, do you think that the code size of the debugInfo function is proportional to its usefulness?

@paulfalgout
Copy link
Collaborator

@jgonggrijp honestly I'd move that debugInfo into a separate directory/module such that people can

import Backbone from 'backbone';
import { debugInfo } from 'backbone/debug-info';

@GammaGames
Copy link

I like that they're <input>s, I think a shorter example would fit the space and help guide the user. And yes, the added code for debugInfo has a lot of value! Gathering all the versions as well as environment would be tedous without it

@jgonggrijp
Copy link
Collaborator Author

Alright, I shortened the placeholder for that <input>, moved debugInfo to a separate module and added documentation for it.

The updated bug report template can be previewed here and the documentation for debugInfo can be previewed here. I would appreciate a final thumbs-up before merging this!

(The linter currently chokes on the ESM syntax in modules/debug-info.js, I'll adress that before merging.)

@GammaGames
Copy link

👍️

Old trick learned from Underscore: within this directory, Node.js and
other tools that follow its awkward convention will be willing to
interpret .js files with ES module syntax.
@jgonggrijp jgonggrijp merged commit 801db14 into jashkenas:master Jan 23, 2024
1 check passed
Dusting off automation moved this from Low priority to Closed Jan 23, 2024
@jgonggrijp jgonggrijp deleted the issue-templates branch January 23, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants