You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/01/28 03:22:44 UTC
[PR] feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary [superset]
mistercrunch opened a new pull request, #32006:
URL: https://github.com/apache/superset/pull/32006
Recently brought in an intricate issue while trying to style the ErrorBoundary. Somehow was triggering a loop of auto-height-expanding.
Here my solution is to pass a css prop to the boundary, which carries through to the `Alert` component
--
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
Re: [PR] feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje closed pull request #32006: feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary
URL: https://github.com/apache/superset/pull/32006
--
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
Re: [PR] feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32006:
URL: https://github.com/apache/superset/pull/32006#discussion_r1931469661
##########
superset-frontend/src/views/App.tsx:
##########
@@ -69,34 +70,36 @@ const LocationPathnameLogger = () => {
return <></>;
};
-const App = () => (
- <Router>
- <ScrollToTop />
- <LocationPathnameLogger />
- <RootContextProviders>
- <GlobalStyles />
- <Menu
- data={bootstrapData.common.menu_data}
- isFrontendRoute={isFrontendRoute}
- />
- <Switch>
- {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
- <Route path={path} key={path}>
- <Suspense fallback={<Fallback />}>
- <Layout.Content>
- <div style={{ padding: '16px' }}>
- <ErrorBoundary>
+const App = () => {
+ const theme = useTheme();
+
+ return (
+ <Router>
+ <ScrollToTop />
+ <LocationPathnameLogger />
+ <RootContextProviders>
+ <GlobalStyles />
+ <Menu
+ data={bootstrapData.common.menu_data}
+ isFrontendRoute={isFrontendRoute}
+ />
+ <Switch>
+ {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
+ <Route path={path} key={path}>
+ <Suspense fallback={<Fallback />}>
+ <Layout.Content>
+ <ErrorBoundary style={{ margin: theme.sizeUnit * 4 }}>
Review Comment:
### Unsafe Theme Property Access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
No type checking or fallback for theme.sizeUnit which could be undefined
###### Why this matters
If theme.sizeUnit is undefined, the multiplication operation will result in NaN, causing unexpected styling behavior.
###### Suggested change ∙ *Feature Preview*
Add a fallback value for theme.sizeUnit:
```tsx
style={{ margin: (theme?.sizeUnit ?? 8) * 4 }}
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dac7017a-96cb-443d-8926-7907059ff387?suggestedFixEnabled=true) and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:464ccbbb-782b-459f-9f21-7061a455528f -->
--
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
Re: [PR] feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #32006:
URL: https://github.com/apache/superset/pull/32006#discussion_r1931480153
##########
superset-frontend/src/views/App.tsx:
##########
@@ -83,13 +83,9 @@ const App = () => (
{routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
<Route path={path} key={path}>
<Suspense fallback={<Fallback />}>
- <Layout.Content>
- <div style={{ padding: '16px' }}>
- <ErrorBoundary>
- <Component user={bootstrapData.user} {...props} />
- </ErrorBoundary>
- </div>
- </Layout.Content>
+ <ErrorBoundary style={{ margin: '16px' }}>
Review Comment:
wanted to do `theme.sizeUnit * 4` but theme isn't in context here so bailed and went for hard-coded
--
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
Re: [PR] feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #32006:
URL: https://github.com/apache/superset/pull/32006#discussion_r1931469450
##########
superset-frontend/src/views/App.tsx:
##########
@@ -69,34 +70,36 @@ const LocationPathnameLogger = () => {
return <></>;
};
-const App = () => (
- <Router>
- <ScrollToTop />
- <LocationPathnameLogger />
- <RootContextProviders>
- <GlobalStyles />
- <Menu
- data={bootstrapData.common.menu_data}
- isFrontendRoute={isFrontendRoute}
- />
- <Switch>
- {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
- <Route path={path} key={path}>
- <Suspense fallback={<Fallback />}>
- <Layout.Content>
- <div style={{ padding: '16px' }}>
Review Comment:
that was the issue here ^^^
--
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
Re: [PR] feat: Fix explore dynamic height issues by adding style to ErrorAlertBoundary [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #32006:
URL: https://github.com/apache/superset/pull/32006#issuecomment-2619143963
I think we can close this PR now as https://github.com/apache/superset/pull/32005 is merged
--
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