feat(settings): add settings page #63

heyhippari · 2020-09-07T19:36:24Z

Desktop

Mobile

sonarcloud · 2020-09-20T01:51:18Z

SonarCloud Quality Gate failed.

5 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

camc314

Looks Great! Only a couple of things...

Something weird is happening here for non-admins:

pages/settings/index.vue

dkanada · 2020-12-08T00:22:46Z

locales/en-US.json

  "noResultsFound": "There is nothing here",
+  "numberTracks": "{number} tracks",
+  "operatingSystem": "Operating system",


Suggested change

"operatingSystem": "Operating system",

"operatingSystem": "OS",

Here's an option that would fit on a single line on mobile devices.

pages/settings/index.vue

ferferga

Also, I think that we should move the assets from assets folder to static (or merge static with assets). Static already contains the images for the PWA manifest so I think it makes sense to move them there.

ferferga · 2020-12-12T12:45:18Z

pages/settings/index.vue

+            <v-row>
+              <v-col>
+                <p class="mb-0">
+                  <span


Why in v-html and not inside the tag? Just like you do in the system version strings. Do we need to keep formatting or something?

This can lead to XSS attacks.

The issue is that we need to translate the link and the text around it. The place of the linked word can change depending on the language, though, which means we need to translate the entire thing, link included.

Of course, the issue is that this opens us to potential issues, but I don't really see another way to do this :/

@mrtimscampi Didn't think about the links, good point.

Not sure if it's worth it, given that no data should be inputted by any users anywhere (but I'm not by any means an XSS expert) but I just had the following idea:

Create a new component that handles this exact situation. We pass it the translation's key string and the component, based on separators, can determine what's a link and what not in the value. For instance:

"Check [Jellyfin](jellyfin.org) docs for more information"

With a computed property, we generate two arrays, one to be used with a for that generates tags and another one for . The tags will be generated from the array ["Check", "docs for more information"] and the from ["[Jellyfin](jellyfin.org)"].

We should be able to cover all the possible cases with a component that parses texts like this while keeping Vetur's complaints away.

Do note that there is sanitation performed on the entire translated chain, using dompurify.

It's explicitly for these kind of cases that I added it to the source.

Vetur's warning is just that: a warning to let you know you need to sanitize whatever you pass to it, which we do already.

@mrtimscampi Fair enough.

The only remnant is the question about the images location 😄

pages/settings/index.vue

camc314 · 2020-12-13T10:48:41Z

pages/settings/index.vue

+                </p>
+              </v-col>
+              <v-col cols="3" class="d-flex justify-end">
+                <v-img src="/icon.png" width="100%" :alt="$t('jellyfinLogo')" />


Suggested change

<v-img src="/icon.png" width="100%" :alt="$t('jellyfinLogo')" />

<v-img

contain

src="/icon.png"

width="100%"

:alt="$t('jellyfinLogo')"

/>

Sometimes logo is cut off. Containing it should fix it

camc314 · 2020-12-13T10:52:36Z

pages/settings/index.vue

+              target="_blank"
+            >


Suggested change

target="_blank"

>

rel="noopener"

target="_blank"

>

camc314 · 2020-12-13T10:53:25Z

locales/en-US.json

+    "helpTranslateLink": "Help <a href=\"{link}\">translate Jellyfin</a> in your language",
+    "poweredByJellyfin": "This server is powered by Jellyfin",
+    "poweredByJellyfinLink": "This server is powered by <a href=\"{link}\">Jellyfin</a>",
+    "readTheDocumentation": "Read the documentation",
+    "readTheDocumentationLink": "Read the <a href=\"{link}\">documentation</a>",
+    "reportAnIssue": "Report an issue with the Vue client",
+    "reportAnIssueLink": "<a href=\"{link}\">Report an issue</a> with the Vue client"


I think it would be better if these links were opened in new tabs

ferferga

Unrelated to my review (and you will hate me for not mentioning this before you did all the design), but:

I think we should make another page for the server settings entirely, so we leave in the main settings page the user options (as is right now) and the bottom section (with a title like Administration) with the first Server button that it's in your screenshots, removing everything else.

That button would route us to another page exclusive for server settings, where the main tab displays the card with server info and the settings that are under General in web. From there, we can switch between the many options that the server has.

This solves the overwhelming feel of the actual design, while also easing configuration (with its own page, we can make two columns, left for pages and right for the settings) without the need of going back and forth for reaching every setting.

Do note that this is based on the screenshots of the first post and I actually haven't checked this out locally, I will provide further comments if necessary once I do

ferferga · 2020-12-17T12:01:46Z

pages/settings/index.vue

+  },
+  methods: {
+    ...mapActions('page', ['setPageTitle', 'setAppBarOpacity']),
+    isEmpty(object: any): boolean {


Suggested change

isEmpty(object: any): boolean {

isEmpty(object: never): boolean {

Let's keep the happiness of the linter :)

heyhippari · 2020-12-17T12:47:10Z

@ferferga This is actually the third or so design that this has had 😋

I've tried to have separate settings and admin settings and the main issue is that there just isn't a nice way to have a tab-based navigation for this.

I followed the Material guidelines as much as I could, because in my opinion, a tabbed navigation makes for a terrible experience on mobile (There's just too many tabs).

Replacing the navigation drawer, as is done on web currently, is also pretty terrible because it doesn't match what any other app does (Yes, Plex has a sidebar on their web client, but their web client isn't responsive and their mobile app is a completely different client, which doesn't allow administration of the server it's connected to).

Ultimately, if we want to be fully responsive, there's some patterns we'll have to follow in order to make the experience at least decent on both platforms. I think this achieves it way better than the current web does or than the other designs I've tried before do, while keeping the line count and complexity as low as possible :)

camc314

Looks good to me. Left some comments wrt the translations

camc314 · 2020-12-17T19:03:11Z

locales/en-US.json

+      "name": "Plugins"
+    },
+    "scheduledTasks": {
+      "description": "Manage scheduled tasks running on your server",


Suggested change

"description": "Manage scheduled tasks running on your server",

"description": "Manage scheduled tasks running on this server",

camc314 · 2020-12-17T19:03:30Z

locales/en-US.json

+      "name": "Transcoding & streaming"
+    },
+    "users": {
+      "description": "Manage your users and their permissions",


Suggested change

"description": "Manage your users and their permissions",

"description": "Manage users and their permissions",

camc314 · 2020-12-17T19:04:08Z

locales/en-US.json

+      "name": "Playback"
+    },
+    "plugins": {
+      "description": "Add and configure new features for your server",


Suggested change

"description": "Add and configure new features for your server",

"description": "Add and configure new features on this server",

camc314 · 2020-12-17T19:04:23Z

locales/en-US.json

+      "name": "Networking"
+    },
+    "notifications": {
+      "description": "Manage and configure notification sent by your server",


Suggested change

"description": "Manage and configure notification sent by your server",

"description": "Manage and configure notification sent by this server",

camc314 · 2020-12-17T19:04:55Z

locales/en-US.json

+      "name": "Media players"
+    },
+    "networking": {
+      "description": "Manage the network settings of your server",


Suggested change

"description": "Manage the network settings of your server",

"description": "Manage the network settings on this server",

camc314 · 2020-12-17T19:05:24Z

locales/en-US.json

@@ -99,8 +184,10 @@
    "mustBeUrl": "This field must be a valid URL",
    "required": "This field is required"
  },
+  "version": "Version",


Suggested change

"version": "Version",

"version": "Server version",

ferferga · 2020-12-17T21:08:12Z

@MrTimscampi This is the concept I had in mind. Please bear in mind with the photoshop aberrations.

Desktop

This is how the main settings page looks:

On the left side, all the options organised by category and with a name (with the proper alignment, size and such, not like in my mockups :P= Whenever something is clicked, on the right side the options are shown, right where it's red. We can add a cool fade effect for that

That's for the user's settings. For server settings, it's all the same, but with one difference: when clicked, left side of the screen fades out and the new server options appear with a fade in effect. Clicking on each option changes what you see on the right side with a fade effect, just like the user options.

Mobile

Same flow, but dividing what's in the right and what's in the left in different pages:

For instance, to reach the server settings, user must follow this route:

Go to general settings (what's shown in the desktop design), which will have the same design as demoed in the first post's descriptions
Click on server: Transition starts and options from the first page are swapped with server options:

Click on API Keys: Transition starts and user will see exactly what you see on the right (red) side of the desktop layout.

Extra things

With this, I think we ensure good responsiveness across screen sizes while not wasting any space in both desktop and mobile. In desktop, we can even put the Jellyfin logo on the right side when there's no option chosen :)

Not demoed in the mockups, but ofc with this layout descriptions of the options will need to change a bit:

Server's button description should be something like "Manage your server", as we're now grouping everything behind that button (As can be seen on desktop mockup, but ofc applies to mobile as well)
While on server settings, first option (can be seen on mobile design) should be something like "Information and General settings", essentially adding there the Architecture, OS, etc + a few basic options (so we don't overcrowd settings page)

heyhippari · 2020-12-17T21:15:44Z

Same flow, but dividing what's in the right and what's in the left in different pages:

That's also one of the things I tried at first, but there's one major blocker: you can't make this responsive. Remember, we have to be able to go from one flow to the other on a resize.

Going from one page to multiple ones isn't exactly resize-friendly.

ferferga · 2020-12-17T21:25:36Z

@MrTimscampi Oh good point. Was thinking only on fixed sizes like mobile phones.

However, we can solve this with an v-show/v-if I think.

We need to make sure that when breakpoint is bigger, we show stuff in the second column. On small breakpoint, we render both the options and values in the first column and toggle it's visibility with v-show or v-if

sonarcloud · 2020-12-17T21:43:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

heyhippari force-pushed the feat/settings branch 2 times, most recently from b3cb5ca to 40ac8a5 Compare September 20, 2020 01:49

heyhippari mentioned this pull request Sep 25, 2020

List of features #115

Closed

90 tasks

heyhippari added this to the Preview Release 1 milestone Dec 6, 2020

heyhippari force-pushed the feat/settings branch from 40ac8a5 to fd353e9 Compare December 6, 2020 17:30

heyhippari marked this pull request as ready for review December 6, 2020 17:34

heyhippari requested a review from camc314 December 6, 2020 17:34

camc314 reviewed Dec 6, 2020

View reviewed changes

pages/settings/index.vue Outdated Show resolved Hide resolved

pages/settings/index.vue Outdated Show resolved Hide resolved

pages/settings/index.vue Show resolved Hide resolved

pages/settings/index.vue Outdated Show resolved Hide resolved

dkanada reviewed Dec 8, 2020

View reviewed changes

pages/settings/index.vue Outdated Show resolved Hide resolved

dkanada reviewed Dec 8, 2020

View reviewed changes

pages/settings/index.vue Outdated Show resolved Hide resolved

camc314 reviewed Dec 9, 2020

View reviewed changes

pages/settings/index.vue Show resolved Hide resolved

heyhippari force-pushed the feat/settings branch from 8718d3a to 5fb4cc3 Compare December 12, 2020 09:51

heyhippari requested a review from camc314 December 12, 2020 09:51

ferferga requested changes Dec 12, 2020

View reviewed changes

heyhippari modified the milestones: Preview Release 1, Preview Release 4 Dec 12, 2020

camc314 reviewed Dec 13, 2020

View reviewed changes

heyhippari added 5 commits December 17, 2020 12:21

feat(settings): add settings page

8239a3f

chore(vscode): add i18nAlly default language

ba43442

fix(settings): make everything translatable

f646ac0

fix(settings): add alt tag to Jellyfin logo

7799d47

fix(settings): fix non-admin page & suggestions

4407243

heyhippari force-pushed the feat/settings branch from ea230f4 to 87d6fd7 Compare December 17, 2020 11:24

refactor(about-links): move about links to a component and reuse

5b8bd28

heyhippari force-pushed the feat/settings branch from 87d6fd7 to 5b8bd28 Compare December 17, 2020 11:37

heyhippari requested review from ferferga and camc314 December 17, 2020 11:41

ferferga requested changes Dec 17, 2020

View reviewed changes

camc314 approved these changes Dec 17, 2020

View reviewed changes

refactor(translation): reword settings translations

f7c6aa0

heyhippari merged commit fad5f7f into master Dec 17, 2020

heyhippari deleted the feat/settings branch December 17, 2020 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(settings): add settings page #63

feat(settings): add settings page #63

heyhippari commented Sep 7, 2020 •

edited

Loading

sonarcloud bot commented Sep 20, 2020

camc314 left a comment

dkanada Dec 8, 2020

dkanada Dec 8, 2020

ferferga left a comment

ferferga Dec 12, 2020

heyhippari Dec 12, 2020

ferferga Dec 12, 2020

heyhippari Dec 12, 2020

ferferga Dec 13, 2020

camc314 Dec 13, 2020

camc314 Dec 13, 2020

camc314 Dec 13, 2020

ferferga left a comment

ferferga Dec 17, 2020

heyhippari commented Dec 17, 2020

camc314 left a comment

camc314 Dec 17, 2020

camc314 Dec 17, 2020

camc314 Dec 17, 2020

camc314 Dec 17, 2020

camc314 Dec 17, 2020

camc314 Dec 17, 2020

ferferga commented Dec 17, 2020

heyhippari commented Dec 17, 2020

ferferga commented Dec 17, 2020

sonarcloud bot commented Dec 17, 2020

	"operatingSystem": "Operating system",
	"operatingSystem": "OS",

	isEmpty(object: any): boolean {
	isEmpty(object: never): boolean {

	"description": "Manage scheduled tasks running on your server",
	"description": "Manage scheduled tasks running on this server",

	"description": "Manage your users and their permissions",
	"description": "Manage users and their permissions",

	"description": "Add and configure new features for your server",
	"description": "Add and configure new features on this server",

	"description": "Manage and configure notification sent by your server",
	"description": "Manage and configure notification sent by this server",

	"description": "Manage the network settings of your server",
	"description": "Manage the network settings on this server",

feat(settings): add settings page #63

feat(settings): add settings page #63

Conversation

heyhippari commented Sep 7, 2020 • edited Loading

sonarcloud bot commented Sep 20, 2020

camc314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferferga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferferga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyhippari commented Dec 17, 2020

camc314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferferga commented Dec 17, 2020

Desktop

Mobile

Extra things

heyhippari commented Dec 17, 2020

ferferga commented Dec 17, 2020

sonarcloud bot commented Dec 17, 2020

heyhippari commented Sep 7, 2020 •

edited

Loading