-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Chore: Add configuration wrapper markdown for the bug report template #10669
Conversation
the change looks good to me. But I'm not sure if the template is still needed. |
What do you mean by "needed"? It's currently linked from https://github.com/eslint/eslint/blob/master/.github/PULL_REQUEST_TEMPLATE.md ( |
we can change the link to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@aladdin-add, Could be, as I think it's identical, but it's not a decision I can take. I could, instead, close this and open another one that does the removal if you guys decide it's the way to go. |
@platinumazure thoughts on #10669 (comment) ? I'm seeing a rationality to remove the identical template! |
I'm okay with removing the duplicate template if it won't change the user experience on a different menu. For example, our New Issue link shows 4(?) different options which link to specific templates- not sure if all of those have to be in the same folder. And then we have the default template, and I don't know if that's used elsewhere. My suggestion would be to create a test repository with the same template setup as our current one, and then make the same change there and see what happens. (Be sure to check on mobile as well, and on tools like GitHub for Windows if they support the use of these templates.) If we see no issues on the test repository, then we can roll it out there. Sound good? |
|
@aladdin-add @not-an-aardvark I'm not clear what the next steps are here. Can this be merged as is, or are changes needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@platinumazure This can be merged as-is. It might be a good idea for us to consider removing this file from |
Merged. Thanks @revolter for contributing! |
Thanks for merging! |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
What changes did you make? (Give an overview)
Copied the wrapper markdown from https://github.com/eslint/eslint/blob/master/.github/ISSUE_TEMPLATE.md.
Is there anything you'd like reviewers to focus on?