You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/08/11 18:33:06 UTC
Re: [PR] feat(theming): add dynamic theme-aware login page background and login form branding [superset]
msyavuz commented on code in PR #34640:
URL: https://github.com/apache/superset/pull/34640#discussion_r2267648631
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -74,7 +74,62 @@ const StyledLabel = styled(Typography.Text)`
`}
`;
+const StyledBackground = styled.div`
+ ${({ theme }) => {
+ const bgImageUrl = theme.loginPageBackgroundImageUrl;
+ const overlayColor =
+ theme.loginPageBackgroundOverlayColor || 'rgba(0, 0, 0, 0.5)';
Review Comment:
How does this default work with dark mode now?
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -74,7 +74,62 @@ const StyledLabel = styled(Typography.Text)`
`}
`;
+const StyledBackground = styled.div`
+ ${({ theme }) => {
+ const bgImageUrl = theme.loginPageBackgroundImageUrl;
+ const overlayColor =
+ theme.loginPageBackgroundOverlayColor || 'rgba(0, 0, 0, 0.5)';
+ const blurAmount = theme.loginPageBackgroundBlur || '0px';
+ return bgImageUrl
+ ? css`
+ position: fixed;
+ top: 0;
+ left: 0;
+ width: 100vw;
+ height: 100vh;
+ z-index: 0;
+ pointer-events: none;
+
+ &::before {
+ content: '';
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-image: url(${bgImageUrl});
+ background-size: cover;
+ background-position: center;
+ filter: blur(${blurAmount});
+ z-index: -1;
+ }
+
+ &::after {
+ content: '';
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-color: ${overlayColor};
+ z-index: 1;
+ }
Review Comment:
Are these psuedo element styles necessary? I think we can simplify this a bit further. Something like this with more tweaks:
```suggestion
background: ${ bgImageUrl ? url(bgImageUrl) : overlayColor }
```
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -74,7 +74,62 @@ const StyledLabel = styled(Typography.Text)`
`}
`;
+const StyledBackground = styled.div`
+ ${({ theme }) => {
+ const bgImageUrl = theme.loginPageBackgroundImageUrl;
+ const overlayColor =
+ theme.loginPageBackgroundOverlayColor || 'rgba(0, 0, 0, 0.5)';
+ const blurAmount = theme.loginPageBackgroundBlur || '0px';
+ return bgImageUrl
+ ? css`
+ position: fixed;
+ top: 0;
+ left: 0;
+ width: 100vw;
+ height: 100vh;
+ z-index: 0;
+ pointer-events: none;
+
+ &::before {
+ content: '';
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-image: url(${bgImageUrl});
+ background-size: cover;
+ background-position: center;
+ filter: blur(${blurAmount});
+ z-index: -1;
+ }
+
+ &::after {
+ content: '';
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-color: ${overlayColor};
+ z-index: 1;
+ }
+ `
+ : '';
+ }}
+`;
+
+const StyledBrandLogo = styled.img`
+ display: block;
+ max-width: 160px;
+ max-height: 64px;
+ margin: 0;
+ object-fit: contain;
+`;
Review Comment:
We do a very similar thing with Main navbar brand logo. Maybe we can extract that out to a component and use that here as well? Also enable us to unify existing brand logo tokens and this new one. (add a config option for displaying brand logo in login for example)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org