Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Revert #10970 to fix filter_box edit control #11051

Merged
merged 1 commit into from
Sep 24, 2020
Merged

fix: Revert #10970 to fix filter_box edit control #11051

merged 1 commit into from
Sep 24, 2020

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Sep 24, 2020 •
edited
Loading

This PR is to reverts #10970 to fix the edit control in filter_box.
please see issue reported here.

cc @etr2460 @zuzana-vej @mistercrunch

@nytai nytai linked an issue Sep 24, 2020 that may be closed by this pull request
PR #10970 breaks the filter box control #11047
Closed
3 tasks
@graceguo-supercat
Copy link
Author

Thanks for everyone's quick response!

@graceguo-supercat graceguo-supercat merged commit 3467d2d into master Sep 24, 2020
@mistercrunch
Copy link
Member

mistercrunch commented Sep 25, 2020 •
edited
Loading

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...

@rusackas
Copy link
Member

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.

@mistercrunch
Copy link
Member

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...

@mistercrunch
Copy link
Member

It does feel like more regressions in Popovers waiting to happen.

amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 27, 2020
Merge remote-tracking branch 'upstream/master' into feat/add_all_dash…
63c3295 
…boards_permissions

* upstream/master: (46 commits)
  fix: surface connection error messages on the client (apache#11077)
  fix(jest): using UTC mock date (apache#11079)
  removing unused component (apache#11072)
  changing to the correct hex color (apache#11073)
  style: remove unecessary padding (apache#11071)
  fix: database list checkboxes (apache#11068)
  feat: adding all icons from the design system to the codebase (apache#11033)
  fix: sql lab autocomplete width (apache#11063)
  clickable labels have outlines, storybook shows them (apache#11034)
  fixed routes for customer in docs (apache#11052)
  Revert "style: fix checkbox color (apache#10970)" (apache#11051)
  feat: add "created by" to dashboard CRUD view (apache#11030)
  Changed `tags.py` and `helpers.py` in `models` module: removed disabled pylint rule `unused_import`, changed unused arguments to private and removed disabled rule `unused-argument. Removed redundant rules.` (apache#11037)
  chore: updated lint rules in models module (apache#11036)
  Removed disable global pytlint rule `standarderror-builtin` which isn't appearing for Python3 (apache#11038)
  Enabled argument-differ for bulk_delete (apache#11039)
  Enabled no-self-use pylint rule in security. Formatter (apache#11041)
  Changed variable name from capitals to lowercase and changed lint rule (apache#11044)
  Revert "ESLint: Re-enable rule default-props-match-prop-types (apache#10868)" (apache#11050)
  feat(saved_queries): add custom api filter for all string & text fields (apache#11031)
  ...

# Conflicts:
#	superset/config.py
#	tests/dashboards/api_tests.py
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 27, 2020
Merge remote-tracking branch 'upstream/master' into feat/add_all_dash…
1ee44c2 
…boards_permissions

* upstream/master: (46 commits)
  fix: surface connection error messages on the client (apache#11077)
  fix(jest): using UTC mock date (apache#11079)
  removing unused component (apache#11072)
  changing to the correct hex color (apache#11073)
  style: remove unecessary padding (apache#11071)
  fix: database list checkboxes (apache#11068)
  feat: adding all icons from the design system to the codebase (apache#11033)
  fix: sql lab autocomplete width (apache#11063)
  clickable labels have outlines, storybook shows them (apache#11034)
  fixed routes for customer in docs (apache#11052)
  Revert "style: fix checkbox color (apache#10970)" (apache#11051)
  feat: add "created by" to dashboard CRUD view (apache#11030)
  Changed `tags.py` and `helpers.py` in `models` module: removed disabled pylint rule `unused_import`, changed unused arguments to private and removed disabled rule `unused-argument. Removed redundant rules.` (apache#11037)
  chore: updated lint rules in models module (apache#11036)
  Removed disable global pytlint rule `standarderror-builtin` which isn't appearing for Python3 (apache#11038)
  Enabled argument-differ for bulk_delete (apache#11039)
  Enabled no-self-use pylint rule in security. Formatter (apache#11041)
  Changed variable name from capitals to lowercase and changed lint rule (apache#11044)
  Revert "ESLint: Re-enable rule default-props-match-prop-types (apache#10868)" (apache#11050)
  feat(saved_queries): add custom api filter for all string & text fields (apache#11031)
  ...

# Conflicts:
#	superset/config.py
#	tests/dashboards/api_tests.py
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@auxten
Revert "style: fix checkbox color (apache#10970)" (apache#11051)
c949277 
This reverts commit e4e78b6.
@amitmiran137 amitmiran137 deleted the revert-10970-checkbox_color branch March 29, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nytai nytai nytai approved these changes

@etr2460 etr2460 etr2460 approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PR #10970 breaks the filter box control
5 participants
@graceguo-supercat @mistercrunch @rusackas @etr2460 @nytai