[explore] Modal to edit chart properties #9051
Conversation
suddjian
changed the title
Codecov Report
@@ Coverage Diff @@
## master #9051 +/- ##
==========================================
- Coverage 59.43% 59.15% -0.29%
==========================================
Files 369 370 +1
Lines 11743 11813 +70
Branches 2884 2899 +15
==========================================
+ Hits 6980 6988 +8
- Misses 4584 4646 +62
Partials 179 179
Continue to review full report at Codecov.
|
| bsSize="sm" | ||
| value={description} | ||
| onChange={event => setDescription(event.target.value)} | ||
| style={{ maxWidth: '100%' }} |
There was a problem hiding this comment.
Inline styles!? Not sure the exact issue this addresses, but we can probably just slap this on all FormControl components via the theme if needed, so it needn't be repeated in the future.
There was a problem hiding this comment.
Perhaps. If you think it's worth messing around with global styles for this specific thing I'll do it. Without that line, you can grab+drag the textarea out into the right column of the modal layout.
| } | ||
|
|
||
| // values of form inputs | ||
| const [name, setName] = useState(slice.slice_name || ''); |
There was a problem hiding this comment.
is there a reason we default to empty string here? Seems like null would make sense since that's what you're sending to the server, and then in the UI we can replace with an empty string if needed.
There was a problem hiding this comment.
I went with empty string since this is used for the state of the <input>, and input values should not be null. Empty strings are converted to null when sending, to allow users to unset fields. It's cleaner this way IMO.
| bsSize="sm" | ||
| value={cacheTimeout} | ||
| onChange={event => | ||
| setCacheTimeout(event.target.value.replace(/[^0-9]/, '')) |
There was a problem hiding this comment.
I think if you made the FormControl's input type "number" then you could get rid of this logic
There was a problem hiding this comment.
That's what I did at first, but I wasn't sure if we wanted to allow non-integers. Do you have any insight into whether that would be ok?
There was a problem hiding this comment.
The marshmallow schema on the endpoint wants an integer
There was a problem hiding this comment.
I suppose that when sending the request, you could do a parseInt on the value to remove the decimal. Maybe that's sufficient then? At least we don't need to run the validator onChange then
There was a problem hiding this comment.
parseInt can return NaN for certain inputs. An input of type="number" allows using "e" and decimal points. I'd prefer not to open the NaN door, personally.
There was a problem hiding this comment.
We could use type="number alongside the regex filter to get the extra little browser widgets that come with number inputs if you think that'd be valuable, but I think the regex does a job here that isn't offered by html.
| owners: owners.map(o => o.value), | ||
| }; | ||
| try { | ||
| const res = await SupersetClient.put({ |
There was a problem hiding this comment.
missed this before, please use complete words for variable names
There was a problem hiding this comment.
👍 Thanks for the review!
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
Adds a menu option to Explore which opens a modal to edit the properties of the chart. This allows changing the chart properties without going to the autogenerated edit page - part of an ongoing effort to deprecate the CRUD pages.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS