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

Update issue templates #5818

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

Conversation

spirillen
Copy link

@spirillen spirillen commented Jul 22, 2024

Changes

Updated the issue templates to support the new yaml format.

This is done to help submitter to type in tedious information such as OS, Browser, Versions as these as easily can be picked from a dropdown optional list.

All fields should have unique identifies following the simple camelCase syntax to make it easier to extract the information via the GH API.

I've kept the old issue styled .md files for reference, and they should be deleted before any merge actually happens.

Please enjoy my small giveback to an awesome project.

Issues

Replacing the tedious of typing the standard values of OS, Browser, Versions

Important

Before actual merging the old issue templates in markdown format need to be deleted or renamed to eg. .old

These have solely been kept as reference of contents in the new yaml styled files

This is done to help submitter to type in tedious information such as OS, Browser, Versions as these as easily can be picked from a dropdown optional list.

All fields should have uniq identifies following the simple camellCase syntax to make it easier to extract the information via the GH API.

I've kept the old issue styled .md files for reference, and they should be deleted before any merge actually happens.

Please enjoy my small giveback to an awsome project.


TODO before merge

  • Remove old markdown issue templates
    • Delete the META template?
  • Adapt current versions, at time of merge

Updated the issue templates to support the new yaml format.

This is done to help submitter to type in tedious information such as OS, Browser, Versions as these as easily can be picked from a dropdown optional list.

All fields should have uniq identifies following the simple calCase syntax to make it easier to extract the information via the GH API.

I've kept the old issue styled .md files for reference, and they should be deleted before any merge actually happens.

Please enjoy my small giveback to an awsome project.
@spirillen spirillen requested a review from a team as a code owner July 22, 2024 13:27
@thornbill
Copy link
Member

Thanks for the contribution!

I haven't reviewed this in depth yet, but we recently updated the server issue template so we should probably make sure this is generally similar. Refs: jellyfin/jellyfin#11135

  • I would also remove versions that do not exist currently. (We will probably automate updating the versions in the template at some point.)
  • I think we can just remove the meta issue template... I think it pretty much only gets (ab)used to not follow the normal issue format.

Copy link

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Aug 16, 2024

Cloudflare Pages deployment

Latest commit 3ec4591
Status ✅ Deployed!
Preview URL https://e5d385a5.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@spirillen
Copy link
Author

Hi @thornbill

I have added a todo list to have these comments in mind before merging. This also means I did not change anything yet, as you mentioned your self, your comments are at first look.

.github/ISSUE_TEMPLATE/1-bug-report.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/1-bug-report.yml Outdated Show resolved Hide resolved
Comment on lines +114 to +119
- 10.10.4
- 10.10.5
- 10.10.6
- 10.10.7
- 10.10.8
- 10.10.9
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- 10.10.4
- 10.10.5
- 10.10.6
- 10.10.7
- 10.10.8
- 10.10.9

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, I would suggest using an input field instead of a dropdown. I personally dislike needing to specify "Other" in a separate field. While I understand it's a GitHub limitation, it's not ideal.

I'd pass this to the Jellyfin-Web crew to decide. Template wise this is totaly okay though!

Copy link
Author

Choose a reason for hiding this comment

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

I would suggest using an input field instead of a dropdown

The choice in the dropdown, is to help avoiding against typo's and tedious typing, that just as well "pre typed" in a dropdown.

The dropdown of versions should of course be maintained by the release script

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, the lack of a dedicated "other" field will break automations, as it won't be possible to reliably extract the contents of this value.

All custom builds from source will be categorized as "other," and all custom browsers will also be labeled as "other." Furthermore, every release will require an update to the dropdown, even if this becomes automated in the future.

If the dropdown included a writable "other" field, the situation would be different, but unfortunately, this is a technical limitation from GitHub.

While the user can specify the value in another field, automations won’t be able to find it. Additionally, this approach adds extra effort, as the user would need to select "other," scroll back up, and manually enter the version information.

Copy link
Author

Choose a reason for hiding this comment

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

@BotBlake I see your point and can follow your POV as wellm Yet not fully convinced it will be a problem.

Chosing to let the question open for other comments, before choosing to fiddle more with this part

Copy link
Member

Choose a reason for hiding this comment

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

I would vote for just using an input also. Our experience shows that unless we have some automation to update the version options, they will quickly get out of date. 😅

Copy link
Author

Choose a reason for hiding this comment

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

🤷🏻‍♂️ You do realize, this is the point, for which I made this template??

Why not make a option for major version.x.x, that opens for code errors?

Comment on lines +21 to +22
description: Please paste any ffprobe or MediaInfo logs.
render: bash
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Please paste any ffprobe or MediaInfo logs.
render: bash
description: Please include any ffprobe or MediaInfo logs.

Copy link
Member

Choose a reason for hiding this comment

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

Afaik, when adding the render option, GitHub will no longer allow a File Upload as a submission in that Text Field.
In the end its a matter of choice, if the team rather wants a bash Codeblock, or a bare Textfield where Users Potentialy Upload logfiles to.

In my personal Oppinion, rendering to bash might the better choice.

Copy link
Author

Choose a reason for hiding this comment

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

The third option is to add a upload log file(s) here

Comment on lines +89 to +95
- 10.10.3
- 10.10.4
- 10.10.5
- 10.10.6
- 10.10.7
- 10.10.8
- 10.10.9
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- 10.10.3
- 10.10.4
- 10.10.5
- 10.10.6
- 10.10.7
- 10.10.8
- 10.10.9
- 10.10.3

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines +46 to +48

render: bash

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
render: bash

Copy link
Author

Choose a reason for hiding this comment

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

Can change the render to logs... I've added the rendering to encapsulate any text that might convert into undesirably links

Comment on lines +3 to +4
projects:
- jellyfin/jellyfin-web
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
projects:
- jellyfin/jellyfin-web

won't work for users without write access to the project in question, at least according to the docs

Copy link
Author

Choose a reason for hiding this comment

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

Leaving for now, as this might change in future

.github/ISSUE_TEMPLATE/2-playback-issue.yml Outdated Show resolved Hide resolved
@oddstr13 oddstr13 changed the title issue template update Update issue templates Dec 21, 2024
spirillen and others added 3 commits December 21, 2024 05:50
options:
- Firefox-ESR
- Firefox
- Chrome based (Chromium, Edge)
Copy link
Member

Choose a reason for hiding this comment

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

Chromium based.
Chrome is based on Chromium. Edge is based on Chromium.
https://en.wikipedia.org/wiki/Chromium_(web_browser)

Words mixed up.

Copy link
Author

Choose a reason for hiding this comment

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

Super, will fix the wording.

Thanks to @BotBlake in jellyfin#5818 (comment) to notice the mix

Signed-off-by: spirillen <[email protected]>
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.

5 participants