Using ContextMenu2 with table rows

[vladimir-djokic]

With ContextMenu I had the ability to use "imperative" API when working with table rows:

Something like...

const handleContextMenu = (rowData: MyData) =>
  (): void => {
    // [1] Do something with rowData, like 'selectMyRow(rowData)' which then changes the rows color...
    // [2] Now show context menu
   ContextMenu.show(...)
};

<tr onContextMenu={handleContextMenu(rowData)}>
</tr>

How can I achieve something like that with ContextMenu2?

If I wrap entire table with ContextMenu2 then I would need to drill down using React/HTML API in order to get the data associated with the row I right-clicked on.

[adidahiya]

did you try this?

const getRowContextMenuHandler = (rowData: MyData) => () => {
  // Do something with rowData, like 'selectMyRow(rowData)' which then changes the rows color...
};
const getRowContextMenu = (rowData: MyData) => {
  // return menu content here
};

return (
  <ContextMenu2 content={getRowContextMenu(rowData)} onContextMenu={getRowContextMenuHandler(rowData)}>
    <tr>{...}</tr>
  </ContextMenu2>
);

[vladimir-djokic]

Given your example, and as I understand <ContextMenu2> from glancing at the source code - won't that produce invalid <table> markup, because each <tr> will be wrapped by <div>?

So instead of (which is valid HTML table markup):

<table>
  <tbody>
    <tr>...</tr> <!--  onContextMenu={handleContextMenu(rowData)} -->
    <tr>...</tr> <!--  onContextMenu={handleContextMenu(rowData)} -->
    <tr>...</tr> <!--  onContextMenu={handleContextMenu(rowData)} -->
    ...
  </tbody>
</table>

Your example would result in something like (which is invalid markup):

<table>
  <tbody>
    <div> <!-- This div is a result of `<ContextMenu2>` -->
      <tr>...</tr>
    </div>
    <div> <!-- This div is a result of `<ContextMenu2>` -->
      <tr>...</tr>
    </div>
    <div> <!-- This div is a result of `<ContextMenu2>` -->
      <tr>...</tr>
    </div>
    ...
  </tbody>
</table>

Also, wrapping each row with <ContextMenu2> fells like a overkill.

[adidahiya]

Ok, then you can try flipping the DOM ordering:

<tr>
  <ContextMenu2 ...>
</tr>

Also, wrapping each row with fells like a overkill.

By what metric? Each context menu has different information about the row, so this is fairly idiomatic React IMO. There may be a separate feature request here for having ContextMenu2 not generate a wrapper element (similar to how Popover2's renderTarget prop lets you do the same), if you really want to simplify the DOM structure.

[vladimir-djokic]

Ok, then you can try flipping the DOM ordering:

<tr>
  <ContextMenu2 ...>
</tr>

Won't that result in invalid HTML markup as well?

<tr>
  <div> <!-- This div is a result of `<ContextMenu2>` -->
    <td>Cell 1</td>
    <td>Cell 2</td>
    <td>Cell 3</td>
  </div>
</tr>

[adidahiya]

Hm, you're right. I will need to spend some more time thinking about this... I'll make sure to figure out a way to support this use case before the final 4.0.0 release.

[adidahiya]

I added a new API in #4635 which should address the problem here, it will allow you to omit the <div>. It will need to be ported from the v4 branch to develop.

[vladimir-djokic]

Thank you! I'm personally fine with this being 4.0 only feature as <ContextMenu> + ContextMenu.show(...) still works fine for 3.x.

[adidahiya]

ported to develop and fixed by #4640

[domehead100]

It is idiomatic React in one way of thinking, but it is also idomatic React that an event handler, such as onContextMenu is handled by a function or method such as ContextMenu.show(...).

Not having an imperative option adds inefficiency in the case of large collections (even in a virtualized table wrapping every cell in a ContextMenu2 increases scrolling jankiness because you're asking the virtual dom to do a lot more work).

I think it's a mistake not to have an imperative option, and seems that they way you've designed it that is truly not an option because the isOpen prop is gone and is only now passed to the content as a read-only prop rather than being controllable from the outside.