-
Notifications
You must be signed in to change notification settings - Fork 15.8k
fix(loading): improve loading screen theming for dark mode support #35129
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
base: master
Are you sure you want to change the base?
Changes from all commits
a7b180d
9617b8b
76c5b40
6f6d925
bd0f146
39d832c
7c00f55
4644dab
7e44735
3732966
00d5ccf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
/* eslint-disable react-prefer-function-component/react-prefer-function-component */ | ||
// eslint-disable-next-line no-restricted-syntax | ||
import React from 'react'; | ||
import { theme as antdThemeImport, ConfigProvider } from 'antd'; | ||
import { theme as antdThemeImport, ConfigProvider, App } from 'antd'; | ||
|
||
// @fontsource/* v5.1+ doesn't play nice with eslint-import plugin v2.31+ | ||
/* eslint-disable import/extensions */ | ||
|
@@ -243,7 +243,7 @@ export class Theme { | |
<ThemeProvider theme={themeState.theme}> | ||
<GlobalStyles /> | ||
<ConfigProvider theme={themeState.antdConfig}> | ||
{children} | ||
<App>{children}</App> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This provides some reset styles that could mess up other parts of the ui. |
||
</ConfigProvider> | ||
</ThemeProvider> | ||
</EmotionCacheProvider> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,13 +317,20 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { | |
|
||
const sortComparator = useCallback( | ||
(a: LabeledValue, b: LabeledValue) => { | ||
// When sortMetric is specified, the backend already sorted the data correctly | ||
// Don't override the backend's metric-based sorting with frontend alphabetical sorting | ||
if (formData.sortMetric) { | ||
return 0; // Preserve the original order from the backend | ||
} | ||
|
||
// Only apply alphabetical sorting when no sortMetric is specified | ||
const labelComparator = propertyComparator('label'); | ||
if (formData.sortAscending) { | ||
return labelComparator(a, b); | ||
} | ||
return labelComparator(b, a); | ||
}, | ||
[formData.sortAscending], | ||
[formData.sortAscending, formData.sortMetric], | ||
Comment on lines
+320
to
+333
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like leftover code from another pr |
||
); | ||
|
||
// Use effect for initialisation for filter plugin | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,6 @@ | |
from superset import event_logger, is_feature_enabled | ||
from superset.daos.dashboard import EmbeddedDashboardDAO | ||
from superset.superset_typing import FlaskResponse | ||
from superset.utils import json | ||
from superset.views.base import BaseSupersetView, common_bootstrap_payload | ||
|
||
|
||
|
@@ -76,7 +75,7 @@ def embedded( | |
dashboard_version="v2", | ||
) | ||
|
||
bootstrap_data = { | ||
extra_bootstrap_data = { | ||
"config": { | ||
"GUEST_TOKEN_HEADER_NAME": current_app.config["GUEST_TOKEN_HEADER_NAME"] | ||
}, | ||
|
@@ -86,10 +85,7 @@ def embedded( | |
}, | ||
} | ||
|
||
return self.render_template( | ||
"superset/spa.html", | ||
return self.render_app_template( | ||
extra_bootstrap_data=extra_bootstrap_data, | ||
entry="embedded", | ||
bootstrap_data=json.dumps( | ||
bootstrap_data, default=json.pessimistic_json_iso_dttm_ser | ||
), | ||
Comment on lines
-92
to
-94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting rid of this will make it so there will be errors when the serialization fails |
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,20 @@ | |
|
||
{% block head_meta %}{% endblock %} | ||
|
||
<style> | ||
body { | ||
background: #fff; | ||
color: #000; | ||
} | ||
|
||
@media (prefers-color-scheme: dark) { | ||
body { | ||
background: #000; | ||
color: #fff; | ||
} | ||
} | ||
</style> | ||
Comment on lines
+33
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant CSS causing style conflicts
Tell me moreWhat is the issue?Inline CSS in the head creates redundant style application that conflicts with existing theme-based styling on the body element. Why this mattersThe body element already has inline styling with theme tokens ( Suggested change ∙ Feature PreviewRemove the inline <body style="margin: 0; padding: 0; background-color: {{ tokens.get('colorBgBase', '#ffffff') }}; color: {{ tokens.get('colorTextBase', '#000000') }};"> Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
||
{% block head_css %} | ||
{% for favicon in favicons %} | ||
<link | ||
|
@@ -69,11 +83,11 @@ | |
{% endblock %} | ||
</head> | ||
|
||
<body {% if standalone_mode %}class="standalone"{% endif %}> | ||
{% set tokens = theme_tokens | default({}) %} | ||
<body {% if standalone_mode %}class="standalone"{% endif %} style="margin: 0; padding: 0; background-color: {{ tokens.get('colorBgBase', '#ffffff') }};"> | ||
|
||
{% block body %} | ||
<div id="app" data-bootstrap="{{ bootstrap_data }}"> | ||
{% set tokens = theme_tokens | default({}) %} | ||
{% set spinner_style = "width: 70px; height: auto; position: absolute; top: 50%; left: 50%; transform: translate(-50%, -50%);" %} | ||
|
||
{% if spinner_svg %} | ||
|
@@ -85,14 +99,10 @@ | |
<!-- Custom URL from theme --> | ||
<img | ||
src="{{ tokens.brandSpinnerUrl }}" | ||
alt="Loading..." | ||
msyavuz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
alt="" | ||
aria-label="Loading" | ||
style="{{ spinner_style }}" | ||
/> | ||
{% else %} | ||
<!-- Fallback: This should rarely happen with new logic --> | ||
<div style="{{ spinner_style }}"> | ||
Loading... | ||
</div> | ||
{% endif %} | ||
</div> | ||
{% endblock %} | ||
|
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.
Should these two use the themed version as well?