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

Improve UX on config page #27

Merged
merged 18 commits into from
May 22, 2022
Merged

Improve UX on config page #27

merged 18 commits into from
May 22, 2022

Conversation

strazto
Copy link
Collaborator

@strazto strazto commented May 13, 2022

Resolves #2

Done:

strazto added 3 commits May 13, 2022 21:58
scope listArgumentsByType to correct form

Select arguments by class instead of type
start making enabled folders checklist-able

Implement save/load folder lists

change enabledfolders to checklist

refactor: rename saveFolderList -> serializeEnabledFolders, fillFolderList -> populateEnabledFolders

refactor listArgumentsByType

start implementing load provider role mapping

implement role mapping

Move remove button markup, change hierachy

remove logging statements

resolve folder population promise before trying to check folders
@strazto strazto marked this pull request as ready for review May 13, 2022 12:04
@strazto
Copy link
Collaborator Author

strazto commented May 13, 2022

@9p4 ready for review

@strazto
Copy link
Collaborator Author

strazto commented May 13, 2022

I request a release be made to the manifest upon this PR's merge

@9p4
Copy link
Owner

9p4 commented May 13, 2022

Sure, I'll put out a new release (maybe not 4.0 though)

This was referenced May 13, 2022
@9p4
Copy link
Owner

9p4 commented May 13, 2022

Looks good, but I would prefer the multi-line textareas to be below the slug instead of inline.

Furthermore, I think there needs to be a tiny bit more info for "Roles" and "Admin Roles" and whether or not the roles are separated by newlines.

@strazto
Copy link
Collaborator Author

strazto commented May 14, 2022

Looks good, but I would prefer the multi-line textareas to be below the slug instead of inline.

I'm not sure what you mean by that, this is what they look like to me

image

Unless you mean the folder role mapping inputs

image

Also, by "slug" do you mean the input labels?

Furthermore, I think there needs to be a tiny bit more info for "Roles" and "Admin Roles" and whether or not the roles are separated by newlines.

good point

Add stylesheet

update markup + styling of specialized forms

improve markup, assign checkboxes as emby-checkbox es

imrpove markup, remove old comments

run stylesheet through prettier

Update help strings
@strazto
Copy link
Collaborator Author

strazto commented May 14, 2022

Role mappings:

image

Help strings:

image

@strazto
Copy link
Collaborator Author

strazto commented May 14, 2022

if it's ready to merge, I'd rather squash 6e198ef into 4668213 , although it's not a dirty commit so if you don't care you can just merge

@9p4
Copy link
Owner

9p4 commented May 15, 2022

This is what I see on Firefox

2022-05-14-20-36-33

@strazto
Copy link
Collaborator Author

strazto commented May 15, 2022

This is what I see on Firefox

![2022-05-14-20-36-33](https://user-images.githubusercontent.com/17993169/168452556-e1772a12-aacb-4c03-87e4-61d103984202.png)

Ew! Looks terrible!

I'll try it on Firefox, I copied the textarea styling over from Jellyfin's, since for some reason they don't expose the textarea stylesheet to plugin pages, and maybe there was something missing

@strazto
Copy link
Collaborator Author

strazto commented May 15, 2022

I don't know how to reproduce your problem on firefox, for me it looks like this:

image

strazto added 5 commits May 15, 2022 13:27
scope listArgumentsByType to correct form

Select arguments by class instead of type
start making enabled folders checklist-able

Implement save/load folder lists

change enabledfolders to checklist

refactor: rename saveFolderList -> serializeEnabledFolders, fillFolderList -> populateEnabledFolders

refactor listArgumentsByType

start implementing load provider role mapping

implement role mapping

Move remove button markup, change hierachy

remove logging statements

resolve folder population promise before trying to check folders
Add stylesheet

update markup + styling of specialized forms

improve markup, assign checkboxes as emby-checkbox es

imrpove markup, remove old comments

run stylesheet through prettier

Update help strings
@strazto
Copy link
Collaborator Author

strazto commented May 15, 2022

It looks fine on all my browsers, it kind of looks like the entire stylesheet isn't being loaded

Check devtools console-

https://github.com/9p4/jellyfin-plugin-sso/pull/27/files#diff-dbce8897198b98e490eac0f35c3bce167a71104bd110d7df7b6c6ae2b12b6cbdR340-R345

Is this code running okay?

Is the stylesheet 404'ing?

I've introduced the stylesheet as another embedded asset, is something going wrong there?

Looks liek your folder lists & role mappings also aren't recieving custom styling, so my best guess is the entire stylesheet isn't making it into your build.

@9p4
Copy link
Owner

9p4 commented May 15, 2022

Seems like the stylesheet isn't loading when Jellyfin is served under a subdirectory.

@9p4
Copy link
Owner

9p4 commented May 15, 2022

Yep that's the issue
2022-05-15-16-25-41

@9p4
Copy link
Owner

9p4 commented May 15, 2022

Should be /jellyfin/web/configurationpage...

Seems like a bug in JF.

The proper URL (/jellyfin/web/configurationpage?name=SSO-Auth.css) loads the stylesheet so the issue is the frontend that isn't adding in the right URL.

@9p4
Copy link
Owner

9p4 commented May 15, 2022

Scratch that, it's a bug in the JS

  addTextAreaStyle: (view) => {
    var style = document.createElement("link");
    style.rel = "stylesheet";
    style.href = "/web/configurationpage?name=SSO-Auth.css";
    view.appendChild(style);
  },

@9p4
Copy link
Owner

9p4 commented May 15, 2022

Alright it seems to be all fixed now.

@9p4
Copy link
Owner

9p4 commented May 15, 2022

Before I merge, there are a few more issues. First, the Folder-Role mapping doesn't have the proper role nor folders.

2022-05-15-16-53-39

Maybe for the default providers, add a hint for what a suitable entry would be. Maybe Jellyfin.Server.Implementations.Users.DefaultAuthenticationProvider?
2022-05-15-16-54-57

@9p4 9p4 mentioned this pull request May 18, 2022
@9p4
Copy link
Owner

9p4 commented May 18, 2022

Just checked and it doesn't seem like the per-provider allowed folders isn't working either (POSTing an empty array, doesn't populate)

@9p4
Copy link
Owner

9p4 commented May 18, 2022

Alright, those issues should all be fixed now. Let me know your thoughts before I merge.

@strazto
Copy link
Collaborator Author

strazto commented May 22, 2022

I'll have a look, but if it's all good from your end, I'm happy.
Just squash merge it if you can, please :)

@strazto
Copy link
Collaborator Author

strazto commented May 22, 2022

LGTM, thanks for fixing those issues up.
I was naughty & never actually tried to save or load folder role mappings, so didn't realize that I had understood the data structure wrong

@9p4 9p4 merged commit f985096 into 9p4:main May 22, 2022
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.

Admin page
2 participants