etr2460 commented on a change in pull request #10426:
URL:
https://github.com/apache/incubator-superset/pull/10426#discussion_r460481241
##########
File path: superset-frontend/src/components/Menu/LanguagePicker.tsx
##########
@@ -17,15 +17,17 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { NavDropdown, MenuItem } from 'react-bootstrap';
-const propTypes = {
- locale: PropTypes.string.isRequired,
- languages: PropTypes.object.isRequired,
-};
+export interface LanguagePickerInterface {
+ locale: string;
+ languages: object;
Review comment:
Instead of object, we prefer `Record<string, any>`. Although in this
case, perhaps we can type this even better, either by looking at what the
backend returns or by looking at the code below. It seems like we might be able
to type this as something like:
```typescript
languages: {
[key: string]: {
flag: string;
url: string;
name: string;
};
};
```
##########
File path: superset-frontend/src/components/Menu/MenuObject.tsx
##########
@@ -51,22 +60,20 @@ export default function MenuObject({ label, icon, childs,
url, index }) {
eventKey={index}
title={navTitle}
>
- {childs.map((child, index1) =>
- child === '-' ? (
+ {childs?.map((child: MenuObjectChildProps | string, index1: number) =>
+ (child as string) === '-' ? (
<MenuItem key={`$${index1}`} divider />
) : (
<MenuItem
- key={`${child.label}`}
- href={child.url}
+ key={`${(child as MenuObjectChildProps).label}`}
+ href={(child as MenuObjectChildProps).url}
eventKey={parseFloat(`${index}.${index1}`)}
>
- <i className={`fa ${child.icon}`} />
- {child.label}
+ <i className={`fa ${(child as MenuObjectChildProps).icon}`} />
+ {(child as MenuObjectChildProps).label}
Review comment:
this might be able to be simpler if we checked if `typeof child ===
'string'`, then we wouldn't need to cast it to a string throughout with `as`.
It'd be safer too
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -17,41 +17,41 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import { Nav, Navbar, NavItem } from 'react-bootstrap';
import styled from '@superset-ui/style';
-import MenuObject from './MenuObject';
+import MenuObject, { MenuObjectProps } from './MenuObject';
import NewMenu from './NewMenu';
import UserMenu from './UserMenu';
import LanguagePicker from './LanguagePicker';
import './Menu.less';
-const propTypes = {
- data: PropTypes.shape({
- menu: PropTypes.arrayOf(PropTypes.object).isRequired,
- brand: PropTypes.shape({
- path: PropTypes.string.isRequired,
- icon: PropTypes.string.isRequired,
- alt: PropTypes.string.isRequired,
- width: PropTypes.oneOfType([PropTypes.string, PropTypes.number])
- .isRequired,
- }).isRequired,
- navbar_right: PropTypes.shape({
- bug_report_url: PropTypes.string,
- version_string: PropTypes.string,
- version_sha: PropTypes.string,
- documentation_url: PropTypes.string,
- languages: PropTypes.object,
- show_language_picker: PropTypes.bool.isRequired,
- user_is_anonymous: PropTypes.bool.isRequired,
- user_info_url: PropTypes.string.isRequired,
- user_login_url: PropTypes.string.isRequired,
- user_logout_url: PropTypes.string.isRequired,
- locale: PropTypes.string.isRequired,
- }).isRequired,
- }).isRequired,
-};
+export interface BrandProps {
+ path: string;
+ icon: string;
+ alt: string;
+ width: string | number;
+}
+export interface NavBarProps {
Review comment:
nit, can you add a newline before this?
also, note to self to make sure this is enforced with a lint rule
##########
File path: superset-frontend/src/components/Menu/LanguagePicker.tsx
##########
@@ -17,15 +17,17 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { NavDropdown, MenuItem } from 'react-bootstrap';
-const propTypes = {
- locale: PropTypes.string.isRequired,
- languages: PropTypes.object.isRequired,
-};
+export interface LanguagePickerInterface {
Review comment:
In general, we should only export interfaces that are actually required
in other files. Let's remove `export` from anything that's not required
elsewhere
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -17,41 +17,41 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import { Nav, Navbar, NavItem } from 'react-bootstrap';
import styled from '@superset-ui/style';
-import MenuObject from './MenuObject';
+import MenuObject, { MenuObjectProps } from './MenuObject';
import NewMenu from './NewMenu';
import UserMenu from './UserMenu';
import LanguagePicker from './LanguagePicker';
import './Menu.less';
-const propTypes = {
- data: PropTypes.shape({
- menu: PropTypes.arrayOf(PropTypes.object).isRequired,
- brand: PropTypes.shape({
- path: PropTypes.string.isRequired,
- icon: PropTypes.string.isRequired,
- alt: PropTypes.string.isRequired,
- width: PropTypes.oneOfType([PropTypes.string, PropTypes.number])
- .isRequired,
- }).isRequired,
- navbar_right: PropTypes.shape({
- bug_report_url: PropTypes.string,
- version_string: PropTypes.string,
- version_sha: PropTypes.string,
- documentation_url: PropTypes.string,
- languages: PropTypes.object,
- show_language_picker: PropTypes.bool.isRequired,
- user_is_anonymous: PropTypes.bool.isRequired,
- user_info_url: PropTypes.string.isRequired,
- user_login_url: PropTypes.string.isRequired,
- user_logout_url: PropTypes.string.isRequired,
- locale: PropTypes.string.isRequired,
- }).isRequired,
- }).isRequired,
-};
+export interface BrandProps {
+ path: string;
+ icon: string;
+ alt: string;
+ width: string | number;
+}
+export interface NavBarProps {
+ bug_report_url?: string;
+ version_string?: string;
+ version_sha?: string;
+ documentation_url?: string;
+ languages: object;
+ show_language_picker: boolean;
+ user_is_anonymous: boolean;
+ user_info_url: string;
+ user_login_url: string;
+ user_logout_url: string;
+ locale: string;
+}
+export interface MenuProps {
Review comment:
nit, can you add a newline before this?
##########
File path: superset-frontend/src/components/Menu/MenuObject.tsx
##########
@@ -17,20 +17,29 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { NavItem, NavDropdown, MenuItem } from 'react-bootstrap';
-const propTypes = {
- label: PropTypes.string.isRequired,
- icon: PropTypes.string.isRequired,
- index: PropTypes.number.isRequired,
- url: PropTypes.string,
- childs: PropTypes.arrayOf(
- PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
- ),
-};
+export interface MenuObjectChildProps {
+ label: string;
+ icon: string;
+ index: number;
+ url?: string;
+}
+export interface MenuObjectProps {
Review comment:
nit: add a new lint above this
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -17,41 +17,41 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import { Nav, Navbar, NavItem } from 'react-bootstrap';
import styled from '@superset-ui/style';
-import MenuObject from './MenuObject';
+import MenuObject, { MenuObjectProps } from './MenuObject';
import NewMenu from './NewMenu';
import UserMenu from './UserMenu';
import LanguagePicker from './LanguagePicker';
import './Menu.less';
-const propTypes = {
- data: PropTypes.shape({
- menu: PropTypes.arrayOf(PropTypes.object).isRequired,
- brand: PropTypes.shape({
- path: PropTypes.string.isRequired,
- icon: PropTypes.string.isRequired,
- alt: PropTypes.string.isRequired,
- width: PropTypes.oneOfType([PropTypes.string, PropTypes.number])
- .isRequired,
- }).isRequired,
- navbar_right: PropTypes.shape({
- bug_report_url: PropTypes.string,
- version_string: PropTypes.string,
- version_sha: PropTypes.string,
- documentation_url: PropTypes.string,
- languages: PropTypes.object,
- show_language_picker: PropTypes.bool.isRequired,
- user_is_anonymous: PropTypes.bool.isRequired,
- user_info_url: PropTypes.string.isRequired,
- user_login_url: PropTypes.string.isRequired,
- user_logout_url: PropTypes.string.isRequired,
- locale: PropTypes.string.isRequired,
- }).isRequired,
- }).isRequired,
-};
+export interface BrandProps {
+ path: string;
+ icon: string;
+ alt: string;
+ width: string | number;
+}
+export interface NavBarProps {
+ bug_report_url?: string;
+ version_string?: string;
+ version_sha?: string;
+ documentation_url?: string;
+ languages: object;
Review comment:
Maybe we can type this better as mentioned above
##########
File path: superset-frontend/src/components/Menu/MenuObject.tsx
##########
@@ -17,20 +17,29 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { NavItem, NavDropdown, MenuItem } from 'react-bootstrap';
-const propTypes = {
- label: PropTypes.string.isRequired,
- icon: PropTypes.string.isRequired,
- index: PropTypes.number.isRequired,
- url: PropTypes.string,
- childs: PropTypes.arrayOf(
- PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
- ),
-};
+export interface MenuObjectChildProps {
+ label: string;
+ icon: string;
+ index: number;
+ url?: string;
+}
+export interface MenuObjectProps {
+ label?: string;
+ icon?: string;
+ index: number;
+ url?: string;
+ childs?: (MenuObjectChildProps | string)[];
Review comment:
I'm not sure if this matches what was in the PropTypes before, can you
validate that it shouldn't be:
```typescript
childs?: MenuObjectChildProps[] | string[];
```
##########
File path: superset-frontend/src/components/Menu/LanguagePicker.tsx
##########
@@ -17,15 +17,17 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { NavDropdown, MenuItem } from 'react-bootstrap';
-const propTypes = {
- locale: PropTypes.string.isRequired,
- languages: PropTypes.object.isRequired,
-};
+export interface LanguagePickerInterface {
Review comment:
can we name this `LangaugePickerProps`, since it's the type for the
props of the react component `LanguagePicker`?
----------------------------------------------------------------
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]
