Skip to content
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

Change MixnodeDetail page's datagrid into a reuseable table component #887

Merged
merged 10 commits into from Nov 12, 2021

Conversation

Aid19801
Copy link
Contributor

@Aid19801 Aid19801 commented Nov 9, 2021

This PR seeks to switch-out the sortable datagrid on Mixnode Detail page for a reuseable MUI Table with one row.

  • created new component <UniversalTable /> using same props as Data Grid for consistency.
  • Flow:
    ➡️ component takes ColumnHeaders (similar to datagrid), maps through and renders out MUI TableHead cells.
    ➡️ component takes preformatted rows prop (parent using util func mixnodeToGridRow()) which in this instance is just one row of data.
    ➡️ component double maps through rows (again, 1 on Mixnode Detail page) AND maps through ColumnsData to match the correct value.
    ➡️ component dynamically checks if the value is bond and formats PUNK, or returns value if not.

MUI / TS THOUGHTS:-

  • I couldn't get export const UniversalTable: React.FC = () => ... to work. Will investigate further but advice where I'm going wrong in understanding that, much appreciated.
  • Is the double .map() alright with you both? Is there any easier way of achieving this? Will explore .reduce and zips shortly and if i find something will push.
  • Spacing: I'd quite like a neater way to space out columns, again tried a couple of approaches (flex: 1 but native mui width overrides it, could pass in width through ColumnsData prop, but feels very manual.)

return val;
}

export const UniversalTable = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a component use React.FC for the typing - Just saw your message above. Will give you a hand getting it to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i figured it out actually. It was the missing angle brackets! 👀

pushed now.

export const UniversalTable: React.FC<{
  tableName: string;
  columnsData: ColumnsType[];
  rows: MixnodeRowType[];
}> = ({ tableName, columnsData, rows }: UniversalTableProps)

<TableBody>
{rows.map((eachRow) => (
<TableRow
key={eachRow.owner}
Copy link
Contributor

Choose a reason for hiding this comment

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

This stops the component from being reuseable as it expects the property owner to exists. I suggest either building it as a specialised component and call it what it is being used for DetailTable or something like that and then speficy the row prop type OR the other option is to make it reuseable by allowing any row properties to be passed to it and using a generic type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to <DetailTable /> for now along with the args for TS.

export interface UniversalTableProps {
tableName: string;
columnsData: ColumnsType[];
rows: any[];
Copy link
Contributor

Choose a reason for hiding this comment

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

'Need this to be typed. See comment below on how we could do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah yeah, beat you to it, sorry. i got there in the end.

@Aid19801 Aid19801 requested review from fmtabbara and removed request for mmsinclair November 11, 2021 15:50
@jstuczyn jstuczyn merged commit 897d51c into develop Nov 12, 2021
@jstuczyn jstuczyn deleted the mixnode-detail-datagrid branch November 12, 2021 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants