[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10426: chore: migrated Menu component to tsx

[GitBox <gi...@apache.org>]

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}`} />
-            &nbsp; {child.label}
+            <i className={`fa ${(child as MenuObjectChildProps).icon}`} />
+            &nbsp; {(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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org