You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/26 15:11:01 UTC

[GitHub] [superset] ayanginet opened a new pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

ayanginet opened a new pull request #13361:
URL: https://github.com/apache/superset/pull/13361


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   Migrated files under `superset-frontend/dashboard/components/menu` from jsx to tsx.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   #### Before and After for MarkdownModeDropdown and PopoverDropdown
   
   <img width="1440" alt="Screen Shot 2021-02-26 at 20 03 21" src="https://user-images.githubusercontent.com/39133100/109317080-23f61e80-786e-11eb-8042-93db0a30e5c3.png">
   <img width="1440" alt="Screen Shot 2021-02-26 at 19 27 28" src="https://user-images.githubusercontent.com/39133100/109317069-1f316a80-786e-11eb-99e7-fbba3f82d81d.png">
   
   #### Before and After for BackgroundStyleDropdown and PopoverDropdown
   
   <img width="1440" alt="Screen Shot 2021-02-26 at 20 03 52" src="https://user-images.githubusercontent.com/39133100/109317365-75061280-786e-11eb-8d42-f98b2f9d2ddd.png">
   <img width="1440" alt="Screen Shot 2021-02-26 at 19 49 26" src="https://user-images.githubusercontent.com/39133100/109317378-79323000-786e-11eb-8eba-0c819b0ad03f.png">
   
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: #10004
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
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

[GitHub] [superset] ayanginet commented on a change in pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
ayanginet commented on a change in pull request #13361:
URL: https://github.com/apache/superset/pull/13361#discussion_r584877081



##########
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:
       I tried to look for when the `key` is used to determine the type. Likely, this is used by antd and the type is `React.Key`.




----------------------------------------------------------------
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

[GitHub] [superset] ayanginet commented on a change in pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
ayanginet commented on a change in pull request #13361:
URL: https://github.com/apache/superset/pull/13361#discussion_r585310779



##########
File path: superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx
##########
@@ -17,44 +17,58 @@
  * 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,
+type ShouldFocusContainer = HTMLDivElement & {
+  contains: (event_target: EventTarget & HTMLElement) => Boolean;
+}
+
+interface WithPopoverMenuProps {
+  children: React.ReactNode,
+  disableClick: Boolean,
+  menuItems: React.ReactNode[],
+  onChangeFocus: (focus: Boolean) => void,
+  isFocused: Boolean,
+  shouldFocus: (event: any, container: ShouldFocusContainer) => Boolean,
+  editMode: Boolean,
+  style: React.CSSProperties,
 };
 
-const defaultProps = {
-  children: null,
-  disableClick: false,
-  onChangeFocus: null,
-  menuItems: [],
-  isFocused: false,
-  shouldFocus: (event, container) =>
-    container?.contains(event.target) ||
-    event.target.id === 'menu-item' ||
-    event.target.parentNode?.id === 'menu-item',
-  style: null,
+interface WithPopoverMenuState {
+  isFocused: Boolean
 };
 
-class WithPopoverMenu extends React.PureComponent {
-  constructor(props) {
+
+export default class WithPopoverMenu extends React.PureComponent<
+  WithPopoverMenuProps,
+  WithPopoverMenuState
+  > {
+
+  container: ShouldFocusContainer;
+
+  static defaultProps = {
+    children: null,
+    disableClick: false,
+    onChangeFocus: null,
+    menuItems: [],
+    isFocused: false,
+    shouldFocus: (event: any, container: ShouldFocusContainer) =>

Review comment:
       I tried to replace the type of the event from `any` to `React.FocusEvent<>` so I could have properties like target. 
   
   However, if I do so, it raises another issue in handleClick(event). Here, both events have to be the same type. In handleClick(event) the function calls document.addEventListener('click', handleClick). addEventListener expects the following types: `(type: string, listener: EventListenerOrEventListenerObject, ...)`.
   
   So basically, I ended up in a situation when I need to choose one from, but if I choose one the other breaks. 
   
   
   
   




----------------------------------------------------------------
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

[GitHub] [superset] codecov[bot] commented on pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #13361:
URL: https://github.com/apache/superset/pull/13361#issuecomment-788742433


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=h1) Report
   > Merging [#13361](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=desc) (f428032) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **decrease** coverage by `2.19%`.
   > The diff coverage is `62.64%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13361/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13361      +/-   ##
   ==========================================
   - Coverage   53.06%   50.86%   -2.20%     
   ==========================================
     Files         489      497       +8     
     Lines       17314    15897    -1417     
     Branches     4482     4065     -417     
   ==========================================
   - Hits         9187     8086    -1101     
   + Misses       8127     7811     -316     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `50.86% <62.64%> (-2.20%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `5.55% <0.00%> (+1.30%)` | :arrow_up: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `54.20% <ø> (-0.26%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `74.66% <0.00%> (-2.05%)` | :arrow_down: |
   | [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `50.00% <ø> (ø)` | |
   | [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
   | [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `84.00% <ø> (-8.00%)` | :arrow_down: |
   | [...ontend/src/components/CertifiedIconWithTooltip.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2VydGlmaWVkSWNvbldpdGhUb29sdGlwLnRzeA==) | `0.00% <ø> (ø)` | |
   | ... and [380 more](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=footer). Last update [4bc2daf...f428032](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [superset] etr2460 commented on a change in pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #13361:
URL: https://github.com/apache/superset/pull/13361#discussion_r585719961



##########
File path: superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx
##########
@@ -17,44 +17,58 @@
  * 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,
+type ShouldFocusContainer = HTMLDivElement & {
+  contains: (event_target: EventTarget & HTMLElement) => Boolean;
+}
+
+interface WithPopoverMenuProps {
+  children: React.ReactNode,
+  disableClick: Boolean,
+  menuItems: React.ReactNode[],
+  onChangeFocus: (focus: Boolean) => void,
+  isFocused: Boolean,
+  shouldFocus: (event: any, container: ShouldFocusContainer) => Boolean,
+  editMode: Boolean,
+  style: React.CSSProperties,
 };
 
-const defaultProps = {
-  children: null,
-  disableClick: false,
-  onChangeFocus: null,
-  menuItems: [],
-  isFocused: false,
-  shouldFocus: (event, container) =>
-    container?.contains(event.target) ||
-    event.target.id === 'menu-item' ||
-    event.target.parentNode?.id === 'menu-item',
-  style: null,
+interface WithPopoverMenuState {
+  isFocused: Boolean
 };
 
-class WithPopoverMenu extends React.PureComponent {
-  constructor(props) {
+
+export default class WithPopoverMenu extends React.PureComponent<
+  WithPopoverMenuProps,
+  WithPopoverMenuState
+  > {
+
+  container: ShouldFocusContainer;
+
+  static defaultProps = {
+    children: null,
+    disableClick: false,
+    onChangeFocus: null,
+    menuItems: [],
+    isFocused: false,
+    shouldFocus: (event: any, container: ShouldFocusContainer) =>

Review comment:
       Hmm, yeah that seems tricky. Maybe add a summary of the issue as a comment, so that way a future engineer knows why `any` was used?




----------------------------------------------------------------
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

[GitHub] [superset] ayanginet commented on a change in pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
ayanginet commented on a change in pull request #13361:
URL: https://github.com/apache/superset/pull/13361#discussion_r583806465



##########
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:
       Yes, because Dropdown does not have such props.




----------------------------------------------------------------
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

[GitHub] [superset] etr2460 commented on pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #13361:
URL: https://github.com/apache/superset/pull/13361#issuecomment-790754274


   Thanks for the contribution!


----------------------------------------------------------------
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

[GitHub] [superset] codecov[bot] edited a comment on pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13361:
URL: https://github.com/apache/superset/pull/13361#issuecomment-788742433






----------------------------------------------------------------
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

[GitHub] [superset] codecov[bot] edited a comment on pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13361:
URL: https://github.com/apache/superset/pull/13361#issuecomment-788742433


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=h1) Report
   > Merging [#13361](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=desc) (0110827) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `4.85%`.
   > The diff coverage is `67.51%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13361/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13361      +/-   ##
   ==========================================
   + Coverage   53.06%   57.91%   +4.85%     
   ==========================================
     Files         489      501      +12     
     Lines       17314    16144    -1170     
     Branches     4482     4148     -334     
   ==========================================
   + Hits         9187     9350     +163     
   + Misses       8127     6794    -1333     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `57.91% <67.51%> (+4.85%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `5.55% <0.00%> (+1.30%)` | :arrow_up: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `54.20% <ø> (-0.26%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `74.66% <0.00%> (-2.05%)` | :arrow_down: |
   | [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `50.00% <ø> (ø)` | |
   | [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
   | [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `92.00% <ø> (ø)` | |
   | [...ontend/src/components/CertifiedIconWithTooltip.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2VydGlmaWVkSWNvbldpdGhUb29sdGlwLnRzeA==) | `0.00% <ø> (ø)` | |
   | ... and [319 more](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=footer). Last update [4bc2daf...12ecfb7](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [superset] etr2460 merged pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #13361:
URL: https://github.com/apache/superset/pull/13361


   


----------------------------------------------------------------
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

[GitHub] [superset] etr2460 commented on a change in pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org



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

[GitHub] [superset] codecov[bot] edited a comment on pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13361:
URL: https://github.com/apache/superset/pull/13361#issuecomment-788742433


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=h1) Report
   > Merging [#13361](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=desc) (12ecfb7) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `3.02%`.
   > The diff coverage is `66.66%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13361/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13361      +/-   ##
   ==========================================
   + Coverage   53.06%   56.09%   +3.02%     
   ==========================================
     Files         489      503      +14     
     Lines       17314    16174    -1140     
     Branches     4482     4157     -325     
   ==========================================
   - Hits         9187     9072     -115     
   + Misses       8127     7102    -1025     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `56.09% <66.66%> (+3.02%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `5.68% <0.00%> (+1.43%)` | :arrow_up: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `54.02% <ø> (-0.44%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `74.66% <0.00%> (-2.05%)` | :arrow_down: |
   | [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `50.00% <ø> (ø)` | |
   | [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
   | [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `92.00% <ø> (ø)` | |
   | [...ontend/src/components/CertifiedIconWithTooltip.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2VydGlmaWVkSWNvbldpdGhUb29sdGlwLnRzeA==) | `0.00% <ø> (ø)` | |
   | ... and [364 more](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=footer). Last update [4bc2daf...12ecfb7](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [superset] codecov[bot] edited a comment on pull request #13361: chore: Migrating dashboard/components/menu from jsx to tsx

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13361:
URL: https://github.com/apache/superset/pull/13361#issuecomment-788742433


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=h1) Report
   > Merging [#13361](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=desc) (12ecfb7) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `4.87%`.
   > The diff coverage is `67.06%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13361/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13361      +/-   ##
   ==========================================
   + Coverage   53.06%   57.93%   +4.87%     
   ==========================================
     Files         489      503      +14     
     Lines       17314    16174    -1140     
     Branches     4482     4157     -325     
   ==========================================
   + Hits         9187     9371     +184     
   + Misses       8127     6803    -1324     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `57.93% <67.06%> (+4.87%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `5.68% <0.00%> (+1.43%)` | :arrow_up: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `54.02% <ø> (-0.44%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `74.66% <0.00%> (-2.05%)` | :arrow_down: |
   | [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `50.00% <ø> (ø)` | |
   | [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
   | [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `92.00% <ø> (ø)` | |
   | [...ontend/src/components/CertifiedIconWithTooltip.tsx](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2VydGlmaWVkSWNvbldpdGhUb29sdGlwLnRzeA==) | `0.00% <ø> (ø)` | |
   | ... and [337 more](https://codecov.io/gh/apache/superset/pull/13361/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=footer). Last update [4bc2daf...12ecfb7](https://codecov.io/gh/apache/superset/pull/13361?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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