etr2460 commented on a change in pull request #13361:
URL: https://github.com/apache/superset/pull/13361#discussion_r583782526
##########
File path: superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx
##########
@@ -121,21 +136,17 @@ class WithPopoverMenu extends React.PureComponent {
style={style}
>
{children}
- {editMode && isFocused && menuItems.length > 0 && (
- <div className="popover-menu">
- {menuItems.map((node, i) => (
- <div className="menu-item" key={`menu-item-${i}`}>
- {node}
- </div>
- ))}
- </div>
- )}
+ {editMode && isFocused && menuItems != undefined
+ && menuItems.length && menuItems.length > 0 && (
Review comment:
I think this could be simpler, something like:
```typescript
editMode && isFocused && (menuItems?.length ?? 0) > 0
```
##########
File path:
superset-frontend/src/dashboard/components/menu/BackgroundStyleDropdown.tsx
##########
@@ -17,35 +17,36 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import cx from 'classnames';
import backgroundStyleOptions from '../../util/backgroundStyleOptions';
-import PopoverDropdown from './PopoverDropdown';
+import PopoverDropdown, { OptionProps } from './PopoverDropdown';
-const propTypes = {
- id: PropTypes.string.isRequired,
- value: PropTypes.string.isRequired,
- onChange: PropTypes.func.isRequired,
+interface BackgroundStyleDropdownProps {
+ id: string,
+ value: string,
+ onChange: Function,
Review comment:
can we define this function with arguments and return types too?
##########
File path: superset-frontend/src/dashboard/components/menu/PopoverDropdown.tsx
##########
@@ -98,8 +104,7 @@ class PopoverDropdown extends React.PureComponent {
const selected = options.find(opt => opt.value === value);
return (
<Dropdown
- id={id}
Review comment:
was removing this intentional?
##########
File path: superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx
##########
@@ -17,44 +17,54 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import cx from 'classnames';
-const propTypes = {
- children: PropTypes.node,
- disableClick: PropTypes.bool,
- menuItems: PropTypes.arrayOf(PropTypes.node),
- onChangeFocus: PropTypes.func,
- isFocused: PropTypes.bool,
- shouldFocus: PropTypes.func,
- editMode: PropTypes.bool.isRequired,
- style: PropTypes.object,
+interface WithPopoverMenuProps {
+ children?: React.ReactNode,
+ disableClick?: Boolean,
+ menuItems?: React.ReactNode[],
+ onChangeFocus?: Function,
+ isFocused?: Boolean,
+ shouldFocus?: Function,
+ editMode: Boolean,
+ style?: Object,
Review comment:
Prefer `Record<string, any>`, or more specific types if possible
##########
File path: superset-frontend/src/dashboard/components/menu/PopoverDropdown.tsx
##########
@@ -111,22 +116,19 @@ class PopoverDropdown extends React.PureComponent {
active: option.value === value,
})}
>
- {renderOption(option)}
+ {renderOption!(option)}
Review comment:
any way to not need the bang here?
##########
File path: superset-frontend/src/dashboard/components/menu/HoverMenu.tsx
##########
@@ -16,23 +16,22 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React from 'react';
-import PropTypes from 'prop-types';
+import React, { RefObject } from 'react';
import cx from 'classnames';
-const propTypes = {
- position: PropTypes.oneOf(['left', 'top']),
- innerRef: PropTypes.func,
- children: PropTypes.node,
+interface HoverMenuProps {
+ position?: 'left' | 'top',
+ innerRef?: RefObject<any>,
Review comment:
we're trying to avoid using `any`, maybe:
- See if you can find the type here
- See if `unknown` works
And apply this principle to all the other cases of `any` too, thanks!
##########
File path: superset-frontend/src/dashboard/components/menu/PopoverDropdown.tsx
##########
@@ -75,20 +71,30 @@ const MenuItem = styled(Menu.Item)`
}
`;
-class PopoverDropdown extends React.PureComponent {
- constructor(props) {
+interface HandleSelectProps {
+ key: any;
Review comment:
this is probably a string or `string | int`?
##########
File path:
superset-frontend/src/dashboard/components/menu/MarkdownModeDropdown.tsx
##########
@@ -17,15 +17,14 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/core';
import PopoverDropdown from './PopoverDropdown';
-const propTypes = {
- id: PropTypes.string.isRequired,
- value: PropTypes.string.isRequired,
- onChange: PropTypes.func.isRequired,
+interface MarkdownModeDropdownProps {
+ id: string,
+ value: string,
+ onChange: Function,
Review comment:
same comment about function types
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
