-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
feat: revamp login page and support disabling media server login #1286
base: develop
Are you sure you want to change the base?
feat: revamp login page and support disabling media server login #1286
Conversation
7a600d5
to
727bd5a
Compare
@@ -97,7 +98,7 @@ function Button<P extends ElementTypes = 'button'>( | |||
if (as === 'a') { | |||
return ( | |||
<a | |||
className={buttonStyle.join(' ')} | |||
className={twMerge(buttonStyle)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tailwind-merge
makes it possible to apply style overrides (like a different background color) without conflicts. I've added it as a dependency and would recommend adopting it generally, since it makes stuff like this a lot more pleasant. In this case, the Plex button would end up with the wrong background color otherwise.
@@ -1,63 +1,39 @@ | |||
import Button from '@app/components/Common/Button'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the Jellyfin setup to its own component (JellyfinSetup.tsx
), since previously the entire JellyfinLogin
component was just an if statement over the initial
flag.
bd62654
to
40c2aca
Compare
I would suggest not to make these changes and only do Right? @gauthier-th |
40c2aca
to
8bce211
Compare
Gotcha. I've gone through and verified most of the translations, so those I'm certain about I would be happy to just submit on weblate instead if that would be better. |
That would be better, yes |
8bce211
to
e3c9048
Compare
The design is gorgeous, good job really! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of the react-transition-group
library?
Can't this be done with TailwindCSS group
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, no. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just a few comments :)
6a00fbc
to
bfbca42
Compare
Update the login screen for better usability, especially with OpenID Connect and Plex login, allowing one-click login and removing the accordion layout. Additionally, ensures that media server login is hidden when disabled in the settings.
bfbca42
to
eb2bd16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I'll do a bit more tests this week-end before we can merge.
@fallenbagel do you want to review?
I can review tonight or tomorrow |
Description
This PR includes enhancements from my original OpenID Connect PR which allowed disabling media server login, as well as additional enhancements to the login page to make it more usable, particularly with Plex and OpenID Connect setups. These changes enable future work on #183, while also addressing some of the concerns from #129. Below is a quick summary of the changes
mediaServerLogin
setting, which can be disabled to remove the option to log in with Plex/Jellyfin/Emby from the login screen.mediaServerLogin
option is disabled.Screenshot (if UI-related)
8mb.video-lKf-jBZAUp3Y.mp4
To-Dos
pnpm build
pnpm i18n:extract
Issues Fixed or Closed