-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Yauheni/76933/table component ts migration #73
Changes from 8 commits
da8446d
05cfc8b
43631a6
4bdb7d0
5a02b94
62f047a
6b3bf44
b22de19
eb5f30c
ba3133c
efc1d90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import Table from './table.jsx'; | ||
import Table from './table'; | ||
import './table.scss'; | ||
|
||
export default Table; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,16 +1,15 @@ | ||||||
import React from 'react'; | ||||||
import classNames from 'classnames'; | ||||||
import PropTypes from 'prop-types'; | ||||||
|
||||||
const Header = ({ children, className }) => ( | ||||||
type THeader = { | ||||||
children: React.ReactNode; | ||||||
className?: string; | ||||||
}; | ||||||
|
||||||
const Header = ({ children, className }: THeader) => ( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for this comment. in other cases it would be great, but here children can’t be optional. in PropsWithChildren children is optional. |
||||||
<div role='rowgroup' className={classNames('dc-table__header', className)}> | ||||||
{children} | ||||||
</div> | ||||||
); | ||||||
|
||||||
Header.propTypes = { | ||||||
children: PropTypes.node.isRequired, | ||||||
className: PropTypes.string, | ||||||
}; | ||||||
|
||||||
export default Header; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,8 +1,13 @@ | ||||||
import React from 'react'; | ||||||
import classNames from 'classnames'; | ||||||
import PropTypes from 'prop-types'; | ||||||
|
||||||
const Row = ({ children, className, has_hover }) => { | ||||||
type TRow = { | ||||||
children: React.ReactNode; | ||||||
className?: string; | ||||||
has_hover?: boolean; | ||||||
}; | ||||||
|
||||||
const Row = ({ children, className, has_hover }: TRow) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
@yauheni-kryzhyk-deriv please also address an issue in packages/p2p/src/components/orders/order-table/order-table-row.jsx where we pass an additional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same is here: children are required |
||||||
return ( | ||||||
<div | ||||||
role='row' | ||||||
|
@@ -15,10 +20,4 @@ const Row = ({ children, className, has_hover }) => { | |||||
); | ||||||
}; | ||||||
|
||||||
Row.propTypes = { | ||||||
children: PropTypes.node.isRequired, | ||||||
className: PropTypes.string, | ||||||
has_hover: PropTypes.bool, | ||||||
}; | ||||||
|
||||||
export default Row; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,7 +147,6 @@ export const icons = | |
'IcCashierPaymentAgent', | ||
'IcCashierPaypalDark', | ||
'IcCashierPaypalLight', | ||
'IcCashierPaymentAgent', | ||
'IcCashierPerfectMoneyDark', | ||
'IcCashierPerfectMoneyLight', | ||
'IcCashierPermatabankDark', | ||
|
@@ -877,4 +876,4 @@ export const icons = | |
'IcWalletZingpayDark', | ||
'IcWalletZingpayLight', | ||
], | ||
}; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yauheni-kryzhyk-deriv this file seems to be extra here😊 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import sample_data from '../sample-data.json'; | |
|
||
const FixedSize = () => ( | ||
<Wrapper is_dark={boolean('Dark Theme', false)}> | ||
<Table fixed scroll_width={400} scroll_height={400}> | ||
<Table fixed scroll_width='400' scroll_height='400'> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yauheni-kryzhyk-deriv please also update packages/components/stories/table/README.md lines 48,49 🙂 |
||
<Table.Header> | ||
<Table.Row className='table-storybook-row'> | ||
{sample_data.fields.map((field, index) => ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -524,7 +524,7 @@ export default class MyProfileStore extends BaseStore { | |
this.setIsConfirmDeleteModalOpen(true); | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yauheni-kryzhyk-deriv do we need to change it in this PR?😊 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t understand this issue, when I delete this space it shows me this deleting like my changes. If I add it, it shows me, that it is also my changes. will leave it with space as it is the same in the code |
||
onSubmit() { | ||
const { general_store } = this.root_store; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason that you didn't use
React.PropsWithChildren
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because children are required here. and children in PropsWithChildren are optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.