You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/09/24 22:57:20 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #11051: fix: Revert #10970 to fix filter_box edit control
graceguo-supercat opened a new pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051
This PR is to reverts apache/incubator-superset#10970.
please see [issue reported here](https://github.com/apache/incubator-superset/issues/11047).
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] mistercrunch commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-699006089
Oh that's brittle. Seems like we need to move 100% away from `Popover` onto antd equivalent, or towards a `SafePopover` that wraps the bootstrap one. Could be done in two steps...
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] mistercrunch commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698722868
Apologies as the person who broke this.
@rusackas any idea why emotion's `<ThemeProvider>` doesn't provide all the way down the tree into that `Popover` 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.
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
[GitHub] [incubator-superset] rusackas commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698735137
> Apologies as the person who broke this.
>
> @rusackas any idea why emotion's `<ThemeProvider>` doesn't provide all the way down the tree into that `Popover` component? I really want to prevent this from happening again...
I haven't figured it out 100% yet, but the appearance to me is that `<Popover>` uses some Jquery-ish DOM manipulation to tack the actual popover elements directly onto the `<body>` tag, and it's effectively outside the scope of React's jurisdiction. I've fixed it in numerous other cases by adding a `<ThemeProvider>` component within the `<Popover>`. For example `AdhocFilterEditPopover` includes:
```
<Popover id="filter-edit-popover" {...popoverProps}>
<ThemeProvider theme={theme}>
<Tabs ...
```
That prevents the breakage, but it does feel a bit ugly. I haven't found a good way to make it more foolproof.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] mistercrunch commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698722868
Apologies as the person who broke this.
@rusackas any idea why emotion's `<ThemeProvider>` doesn't provide all the way down the tree into that `Popover` 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.
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
[GitHub] [incubator-superset] mistercrunch edited a comment on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
mistercrunch edited a comment on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698722868
Apologies as the person who broke this.
@rusackas any idea why emotion's `<ThemeProvider>` doesn't provide all the way down the tree into that `Popover` component? I really want to prevent this from happening again...
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] graceguo-supercat commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698630762
Thanks for everyone's quick response!
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] mistercrunch commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-699006486
It does feel like more regressions in Popovers waiting to happen.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] graceguo-supercat commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698630762
Thanks for everyone's quick response!
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] mistercrunch edited a comment on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
mistercrunch edited a comment on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698722868
Apologies as the person who broke this.
@rusackas any idea why emotion's `<ThemeProvider>` doesn't provide all the way down the tree into that `Popover` component? I really want to prevent this from happening again...
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] rusackas commented on pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051#issuecomment-698735137
> Apologies as the person who broke this.
>
> @rusackas any idea why emotion's `<ThemeProvider>` doesn't provide all the way down the tree into that `Popover` component? I really want to prevent this from happening again...
I haven't figured it out 100% yet, but the appearance to me is that `<Popover>` uses some Jquery-ish DOM manipulation to tack the actual popover elements directly onto the `<body>` tag, and it's effectively outside the scope of React's jurisdiction. I've fixed it in numerous other cases by adding a `<ThemeProvider>` component within the `<Popover>`. For example `AdhocFilterEditPopover` includes:
```
<Popover id="filter-edit-popover" {...popoverProps}>
<ThemeProvider theme={theme}>
<Tabs ...
```
That prevents the breakage, but it does feel a bit ugly. I haven't found a good way to make it more foolproof.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] graceguo-supercat merged pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] graceguo-supercat merged pull request #11051: fix: Revert #10970 to fix filter_box edit control
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #11051:
URL: https://github.com/apache/incubator-superset/pull/11051
----------------------------------------------------------------
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.
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