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.

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

fix: Cannot add control from Controls menu with click #1363

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Jun 8, 2023

  • We were defaulting the value to null instead of undefined
  • A golden-layout refactor explicitly checked for undefined instead of null
  • For some reason the type error in FilterPlugin and also AppMainContainer wasn't throwing an error when building?? Will need to investigate why not
  • Loosen restriction to just be != null instead of !== undefined as that is the intent
  • Fixes Cannot add Control from Controls menu with click #1362

- We were defaulting the value to `null` instead of `undefined`
- A golden-layout refactor explicitly checked for `undefined` instead of `null`
- For some reason the type error in `FilterPlugin` and also `AppMainContainer` wasn't throwing an error when building?? Will need to investigate why not
- Loosen restriction to just be `!= null` instead of `!== undefined` as that is the intent
@mofojed mofojed requested review from dsmmcken and mattrunyon June 8, 2023 20:32
@mofojed mofojed self-assigned this Jun 8, 2023
@dsmmcken
Copy link
Contributor

dsmmcken commented Jun 8, 2023

Is it the same event that is erroring if I look at GridPlugin.tsx?

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #1363 (c7e8ffc) into main (d1ce140) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #1363   +/-   ##
=======================================
  Coverage   45.95%   45.95%           
=======================================
  Files         494      494           
  Lines       34423    34422    -1     
  Branches     8587     8586    -1     
=======================================
  Hits        15820    15820           
+ Misses      18552    18551    -1     
  Partials       51       51           
Flag Coverage Δ
unit 45.95% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/code-studio/src/main/AppMainContainer.tsx 35.13% <0.00%> (+0.11%) ⬆️
...ckages/dashboard-core-plugins/src/FilterPlugin.tsx 28.30% <0.00%> (ø)
packages/dashboard/src/layout/LayoutUtils.ts 13.94% <0.00%> (ø)

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works but event typing around this is messed up.

@mofojed mofojed merged commit 65c0925 into deephaven:main Jun 8, 2023
@mofojed mofojed deleted the 1362-fix-controls-menu branch June 8, 2023 21:01
@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot add Control from Controls menu with click
2 participants