[DX-720] Enable nested tables to contain multiple types#3186
[DX-720] Enable nested tables to contain multiple types#3186
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
m-hulbert
force-pushed
the
dx-720-nested-multiple-types
branch
from
7285a58 to
780c960
Compare
aralovelace
left a comment
aralovelace
left a comment
There was a problem hiding this comment.
hi @m-hulbert just few feedback and suggestion. let me know what you think?
| | Parameter | Required | Description | Type | | ||
| | --- | --- | --- | --- | | ||
| | data | Optional | JSON-serializable data to associate with the user's presence. | <Table id='PresenceData'/> or String | | ||
| | data | Optional | JSON-serializable data to associate with the user's presence. | <Table id='PresenceEvent'/> or <Table id='IsUserPresentParameters'/> or <Table id='PresenceMember'/> | |
There was a problem hiding this comment.
I made this data up for testing - will remove the file before I merge 🙂
| | Parameter | Required | Description | Type | | ||
| | --- | --- | --- | --- | | ||
| | events | Required | The event type(s) to subscribe to. | <Table id='PresenceEventType'/> or PresenceEventType[] | | ||
| | listener | Required | Callback function invoked when matching presence events occur. | (event: <Table id='PresenceEvent'/>) =\> void | |
There was a problem hiding this comment.
wondering if this =\> does it need to be => ?
There was a problem hiding this comment.
Yep you're right but this was just mock data to test it for now.
| {/* Expanded: button attached to nested container */} | ||
| {refExpanded && ( | ||
| <div className="mt-3 border border-neutral-400 dark:border-neutral-900 rounded-lg overflow-hidden"> | ||
| {/* Hide button as header of the container */} |
There was a problem hiding this comment.
would the comment should be Show button ? since its a show component below?
| </div> | ||
| ))} | ||
| return ( | ||
| <React.Fragment key={id}> |
There was a problem hiding this comment.
does the table need to add a bit of spaces on the top ? or this is ok?
<div className="mt-2"> {/* Add wrapper with margin */}
<NestedTableExpandButton ... />
</div>
There was a problem hiding this comment.
I think it's ok - do you think it needs a bit more?
m-hulbert
requested a deployment
to
ably-docs-dx-720-nested-nlmytf
4 participants
Description
The nested table component introduced in #3130 didn't allow for multiple tables to be reference in a cell, nor for a table + text when a property could be of different types.
This PR adds that functionality and also adds a unit test for nested tables in general.
You can temporarily test it here.
Checklist