Add web path as config option #172
Conversation
src/dashboard/dashboardgeneral.js
Outdated
| } | ||
| picker.close(); | ||
| }, | ||
| validateWriteable: true, |
|
I agree with @JustAMan, the server-side change is something for packaging and really advanced users, not something that should be configurable within the UI itself. |
|
I still think you should be able to edit this path along with all the others, makes no sense to hide an option from the dashboard just because it may be for advanced users. I did add a warning to the description, but I could even trigger a modal first with an explanation similar to the plugin installation message. |
src/strings/en-us.json
Outdated
| @@ -1123,7 +1123,7 @@ | |||
| "LabelMetadataPath": "Metadata path:", | |||
| "LabelMetadataPathHelp": "Specify a custom location for downloaded artwork and metadata.", | |||
| "LabelWebPath": "Web path:", | |||
| "LabelWebPathHelp": "The path where the web client source is located.", | |||
| "LabelWebPathHelp": "The path where the web client source is located. Do not change this unless you plan on moving the web files, or the web interface will break.", | |||
There was a problem hiding this comment.
I would rather we not let users edit this via web UI (warning text is something no one would actually read).
|
Changing the ui path feels more like a startup option. What's the use case exactly? |
|
So to clarify based on Riot chats around this - I like the idea of showing the web path, but not the idea of it being changeable from the UI - too much potential for a chicken-and-egg problem there if someone typos it. @dkanada if you can adjust it to just be for show I approve. |
|
Discussed more on Riot with @dkanada. I can see the usecase of setting this from the GUI for quick testing across all paltforms. Assuming it actually does save the configured value in @JustAMan while I understand the argument, I think the potential for additional admin configurability outweighs the possible danger as long as the warning is clear enough. If the user clicks through a big warning and breaks it, OK, we can put a blurb in the docs. |
|
I just removed it for now, the visible section on the dashboard is working fine though so this can be merged. |
There was a problem hiding this comment.
LGTM.
As for modifying the thing from UI for testing (as @dkanada said in Matrix) - I see the point that it eases the tests, but it's not that frequent - one can just have two instances running on different ports.
So in my view the danger outweighs the usability improvement, but if we move it to "Advanced"/"Experts"/"Developer" part of the UI I won't be objecting too loud.
| @@ -1329,6 +1329,7 @@ | |||
| "LabelVideoCodec": "Video: {0}", | |||
| "LabelVideoType": "Video Type:", | |||
| "LabelView": "View:", | |||
| "LabelWeb": "Web", | |||
There was a problem hiding this comment.
| "LabelWeb": "Web", | |
| "LabelWeb": "Web UI", |
maybe?
related to jellyfin/jellyfin#1092