rusackas commented on PR #35897:
URL: https://github.com/apache/superset/pull/35897#issuecomment-3874641366
Thanks for this feature! ReviewBot3000 reporting in...
A couple of things to address before we can merge:
## 1. Form State Sync Issue
The checkbox handlers (setToAllRow, setToTextColor, etc.) update local
React state but don't call
form.setFieldsValue() to sync the Form's internal state. This can cause
the form to submit stale
values. For example:
```
// Current pattern - only updates local state
onChange={event => setToAllRow(event.target.checked)}
// Should also sync form state
onChange={event => {
const checked = event.target.checked;
setToAllRow(checked);
form.setFieldsValue({ toAllRow: checked });
}}
```
## 2. Mixed Naming Patterns
The PR introduces objectFormatting (enum-based) alongside the existing
toAllRow/toTextColor
(boolean) patterns. This creates two ways to express the same concept:
```
// New pattern
formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR
// Old pattern still used
formatter.toTextColor
```
Could we consolidate on one approach? Either:
- Migrate fully to objectFormatting and deprecate the boolean flags, or
- Keep the boolean flags and skip the new enum
This will make the code easier to maintain and avoid confusion about which
pattern to check.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
