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

Readme correction #322

Merged
merged 3 commits into from
Dec 22, 2024
Merged

Conversation

morpheus133
Copy link
Contributor

Fixes

  • README Bug: Corrected the file exclusion for building the web app. gulpfile.js was incorrectly excluded instead of gulpfile.babel.js. (Fix Commit)

Improvements

  • Added Latest Version: Updated the README to include the latest release version (release-10.10.z). (Improvement Commit)

  • Split Build Instructions: Separated build instructions for Jellyfin Web 10.8- and 10.9+ for clarity and ease of use. (Improvement Commit)

Copy link
Collaborator

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

  1. Name the commit 1 as a6a72c8
Update Jellyfin Web branch

or

Update Jellyfin Web branch to 10.10.z
  1. Name the commit 2 as
Split build instructions for Jellyfin Web 10.8- and 10.9+

Simplified the process by splitting the instructions for easier copying.
Included steps for 10.8- for completeness, despite its likely lower usage.

Tbf, I'm not sure about the log message (long part) and whether this splitting is even necessary (because this also requires fixing the Windows wiki, which has Command line and PowerShell commands).

  1. Name the commit 3 as
Exclude gulpfile from packaging

gulpfile.js was renamed in 6a6a14cb9b3a6016db0a469aa3f14bdfc8a861d0

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dmitrylyzo dmitrylyzo added the documentation Improvements or additions to documentation label Dec 19, 2024
@morpheus133
Copy link
Contributor Author

Tbf, I'm not sure about the log message (long part) and whether this splitting is even necessary (because this also requires fixing the [Windows wiki](https://github.com/jellyfin/jellyfin-tizen/wiki/Build-on-Windows#build-jellyfin-web), which has Command line and PowerShell commands).

Updating the wiki is necessary because of the gulpfile.
While we’re at it, I’d add this as well.
Here is the PR for that:
#324

@dmitrylyzo
Copy link
Collaborator

Updating the wiki is necessary because of the gulpfile.

Yes, but what I meant was, is the splitting 10.8/10.9 really worth?
SKIP_PREPARE simply has no effect on 10.9+, and USE_SYSTEM_FONTS has no effect on 10.8-, so you can use one set of commands for both.
Also, as you note, 10.8- is unlikely to be used in the future. Moreover, we always drop support of the previous major release (10.9 after 10.10 is out). So maybe we should just remove everything related to 10.8-.

If we are going to continue splitting command sets, I think the remark blocks are a bit blended.
We could wrap them with collapsible blocks:

mockup begin

Build Jellyfin Web

For version 10.9+:
cd jellyfin-web
npm ci --no-audit
USE_SYSTEM_FONTS=1 npm run build:production

USE_SYSTEM_FONTS=1 is required to discard unused fonts and to reduce the size of the app. (Since Jellyfin Web 10.9)

For version 10.8-:
cd jellyfin-web
SKIP_PREPARE=1 npm ci --no-audit
npm run build:production

SKIP_PREPARE=1 is needed for version 10.8-.

You should get jellyfin-web/dist/ directory.

Use npm run build:development if you want to debug the app.

If any changes are made to jellyfin-web/, the jellyfin-web/dist/ directory will need to be rebuilt using the command above.

mockup end

It is still not perfect, but 🤷

@morpheus133
Copy link
Contributor Author

... Also, as you note, 10.8- is unlikely to be used in the future. Moreover, we always drop support of the previous major release (10.9 after 10.10 is out). So maybe we should just remove everything related to 10.8-.

Than the easiest is to remove it.
10.10 is working with tizen so no reason to keep/use older versions.
If somebody need it than check it in the README history.

@dmitrylyzo dmitrylyzo merged commit 5760c2c into jellyfin:master Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants