mistercrunch commented on code in PR #32980:
URL: https://github.com/apache/superset/pull/32980#discussion_r2029059793
##########
superset-frontend/src/components/Checkbox/Checkbox.tsx:
##########
@@ -16,43 +16,29 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { CSSProperties } from 'react';
-import { styled } from '@superset-ui/core';
-import { CheckboxChecked, CheckboxUnchecked } from 'src/components/Checkbox';
+import { Checkbox as AntCheckbox } from 'antd-v5';
+import { ReactNode } from 'react';
export interface CheckboxProps {
checked: boolean;
onChange: (val?: boolean) => void;
- style?: CSSProperties;
- className?: string;
+ disabled?: boolean;
+ children?: ReactNode;
}
-const Styles = styled.span`
- &,
- & svg {
- vertical-align: top;
- }
-`;
-
export default function Checkbox({
checked,
onChange,
- style,
- className,
+ disabled,
+ children,
}: CheckboxProps) {
return (
- <Styles
- style={style}
- onClick={() => {
- onChange(!checked);
- }}
- role="checkbox"
- tabIndex={0}
- aria-checked={checked}
- aria-label="Checkbox"
- className={className || ''}
+ <AntCheckbox
+ checked={checked}
+ onChange={e => onChange(e.target.checked)}
+ disabled={disabled}
Review Comment:
Yes, are we simply re-declaring props and passing them as is? I'm a big
proponent of using components "as provided" from antd, unless we either:
- want to add useful / commonly used functionality that we'd otherwise have
to repeat in the parent components. An example of that is if our designers
suggested a fancy animation that all of our checkboxes should use consistently
everywhere in the app.
- we want to hide certain functionality that we decide is a no-no
design-wise in Superset. I guess a bad example of that would say if there were
builtin danger/warning/info checkboxes and our designers said "we should never
use those, they're bad design, shouldn't be in our design lexicon...."
Also want to note that "indirection" is annoying and reduces devex/velocity.
Say if we decided to rename `checked` to `marked` for some reason, it means
that instead of being able to use the rich antd documentation and logic (which
some may have memorized from working on other antd projects), then you have to
look up our own component lib, maybe refer to the code, ... which is
inefficient and doesn't add value.
--
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]
