FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore(lint): eslint react-table by evwilkin · Pull Request #3693 · patternfly/patternfly-react · GitHub

chore(lint): eslint react-table - #3693

Merged
redallen merged 29 commits into
patternfly:masterfrom
evwilkin:fix/eslint-tables
Feb 10, 2020
Merged

chore(lint): eslint react-table#3693
redallen merged 29 commits into
patternfly:masterfrom
evwilkin:fix/eslint-tables

Conversation

evwilkin commented Feb 6, 2020

Copy link
Copy Markdown
Member

What: Towards #3592 - linting for react-table

patternfly-build commented Feb 6, 2020
edited
Loading

Copy link
Copy Markdown
Collaborator

codecov-io commented Feb 6, 2020
edited
Loading

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@54a4835). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##             master    #3693   +/-   ##
=========================================
  Coverage          ?   71.26%           
=========================================
  Files             ?      779           
  Lines             ?    10489           
  Branches          ?     2261           
=========================================
  Hits              ?     7475           
  Misses            ?     2592           
  Partials          ?      422
Flag Coverage Δ
#misc 95.45% <ø> (?)
#patternfly3 85.89% <0%> (?)
#patternfly4 60.2% <100%> (?)
Impacted Files Coverage Δ
...ly-4/react-core/src/components/Card/CardFooter.tsx 100% <ø> (ø)
...nfly-4/react-core/src/components/Card/CardBody.tsx 100% <ø> (ø)
...act-core/src/components/DataList/DataListCheck.tsx 30% <ø> (ø)
...tternfly-4/react-core/src/components/Card/Card.tsx 100% <ø> (ø)
...3/patternfly-react/src/components/Slider/Slider.js 89.28% <0%> (ø)
...ct-core/src/components/DataList/DataListAction.tsx 84.61% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54a4835...f8e4475. Read the comment docs.

rowKey = 'id' as string,
onRowClick = (event: React.MouseEvent, row: IRow, rowProps: IExtraRowData, computedData: IComputedData) => undefined as OnRowClick,
/* eslint-disable @typescript-eslint/no-unused-vars */
onRow = (...args: any) => Object,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

onRow and onRowClick appear to be used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Both onRow and onRowClick were throwing TS errors - it looks like the unused variable errors were for the arguments to both functions (...args for onRow, and event/row/rowProps/computedData for onRowClick)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we track down how onRow is used and give it a proper type?

Also, for the unused variables, just put underscores before the variable names. Like _event: React.MouseEvent. Then you can remove the eslint-disable.

evwilkin requested a review from redallen February 7, 2020 15:24
rowKey = 'id' as string,
onRowClick = (event: React.MouseEvent, row: IRow, rowProps: IExtraRowData, computedData: IComputedData) => undefined as OnRowClick,
/* eslint-disable @typescript-eslint/no-unused-vars */
onRow = (...args: any) => Object,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we track down how onRow is used and give it a proper type?

Also, for the unused variables, just put underscores before the variable names. Like _event: React.MouseEvent. Then you can remove the eslint-disable.

class BaseBody extends React.Component<BodyProps, {}> {
static defaultProps = {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
onRow: (...args: any) => Object

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A proper type for onRow would be wonderful.

dlabrecq previously approved these changes Feb 10, 2020
redallen previously approved these changes Feb 10, 2020

redallen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's fine that we can't solve the onRow prop for now. We can fix it in the future.

redallen changed the title Fix/eslint tables chore(lint): eslint tables Feb 10, 2020
redallen changed the title chore(lint): eslint tables chore(lint): eslint react-table Feb 10, 2020
evwilkin dismissed stale reviews from redallen and dlabrecq via 7985b41 February 10, 2020 16:27
dlabrecq previously approved these changes Feb 10, 2020
evwilkin requested a review from dlabrecq February 10, 2020 18:45
redallen merged commit e628ae1 into patternfly:master Feb 10, 2020

Copy link
Copy Markdown
Collaborator

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.3.12
  • @patternfly/react-core@3.136.9
  • @patternfly/react-docs@4.18.14
  • @patternfly/react-inline-edit-extension@2.16.11
  • demo-app-ts@3.22.9
  • @patternfly/react-table@2.26.11
  • @patternfly/react-topology@2.13.11
  • @patternfly/react-virtualized-extension@1.3.107

Thanks for your contribution! 🎉

}

export interface IHeaderRow extends ColumnType { }
export type IHeaderRow = ColumnType;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we have changed this from interface to type?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL