fix: always pass a string as a value to ace editor #13563
Conversation
eschutho
changed the title
eschutho
force-pushed
the
elizabeth/fix-template-params
branch
from
8691e63
to
a5e9b72
Compare
|
Tested locally and learned something new today, LGTM! Thanks for the fix! 🙏 @eschutho Functionality ✅ Screen.Recording.2021-03-11.at.5.09.08.PM.mov@mistercrunch is there a reason why this feature behind a FF? it's working pretty nicely now. |
eschutho
force-pushed
the
elizabeth/fix-template-params
branch
from
a5e9b72
to
1859027
Compare
|
flaky test? |
Codecov Report
@@ Coverage Diff @@
## master #13563 +/- ##
==========================================
- Coverage 77.35% 76.29% -1.07%
==========================================
Files 909 910 +1
Lines 46118 46310 +192
Branches 5649 5613 -36
==========================================
- Hits 35674 35330 -344
- Misses 10309 10816 +507
- Partials 135 164 +29
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -150,7 +151,8 @@ export default function AsyncAceEditor( | |||
| mode={mode} | |||
| theme={theme} | |||
| tabSize={tabSize} | |||
| {...props} | |||
| defaultValue={defaultValue} | |||
| {...{ ...props, value: props.value || '' }} | |||
There was a problem hiding this comment.
The syntax here looks alien. 😄
Why not:
| {...{ ...props, value: props.value || '' }} | |
| {...props} | |
| value={props.value || '' } |
There was a problem hiding this comment.
ha, yeah, that was my first implementation, but often people like to see the "rest" props at the end, and so I anticipated someone changing this order in the context of "cleanup". I think I found a way around this.. lmk what you think.
| }: { | ||
| code: string; | ||
| language: 'yaml' | 'json'; | ||
| onChange: (...args: any) => any; |
There was a problem hiding this comment.
how about this?
| onChange: (...args: any) => any; | |
| onChange: () => void; |
There was a problem hiding this comment.
When we are convert SqlEditor.jsx to the typescript, we can add function parameter type again.
superset/superset-frontend/src/SqlLab/components/SqlEditor.jsx
Lines 512 to 518 in d439da2
There was a problem hiding this comment.
We should be getting to it soon, and then, yes we'll be able to manage types of props better, ya!
|
🏷 ready-to-merge |
* always pass a string as a value to ace editor * fix some syntax (cherry picked from commit 67ffea8)
* always pass a string as a value to ace editor * fix some syntax (cherry picked from commit 67ffea8)
* always pass a string as a value to ace editor * fix some syntax
mistercrunch
added
🏷️ bot
SUMMARY
Fixes issue with template params modal not opening when value was null.
I converted one file to tsx so that we can put a null check on the code argument in TemplateParamsEditor, but it won't help much until the rest of the files are converted. I also made the initial state undefined instead of null so that we can use default args downstream. Lastly, I put in a check at the ace editor itself to never accept a null or undefined value, but pass an empty string in that case.
fixes #13540
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
see #13540 for before state (no modal opens)
after:
TEST PLAN
unit tests aren't completely reliable when it comes to passing a null value to ace editor. It was difficult to reproduce the issue in the tests. I wrote tests around the new code to ensure that we are using undefined instead of null.
ADDITIONAL INFORMATION