feat: Picker - Item description support #1855
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1855 +/- ##
==========================================
- Coverage 46.12% 46.11% -0.02%
==========================================
Files 635 636 +1
Lines 38025 38044 +19
Branches 9612 9623 +11
==========================================
+ Hits 17540 17544 +4
- Misses 20432 20447 +15
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bmingles
force-pushed
the
293-picker-table-support
branch
from
a709176
to
b746685
Compare
| content = content.map((item, i) => | ||
| isElementOfType(item, Text) | ||
| ? cloneElement(item, { | ||
| ...item.props, | ||
| // `cloneElement` has the side effect of resetting React's internal | ||
| // `_store.validated` value to `false on the item. This causes it | ||
| // to be re-validated as a child in an array when is is rendered, | ||
| // even if the item was originally provided as an inline child. | ||
| // Since React expects array children to have explicit keys, this | ||
| // will show devtools warnings for items that wouldn't usually | ||
| // require explicit keys. Since we are only cloning `Text` nodes, it | ||
| // should be reasonable to fallback to a key matching the stringified | ||
| // content. The index suffix is an extra precation for when 2 <Text> | ||
| // nodes have the same value. | ||
| key: item.key ?? `${item.props.children}_${i}`, | ||
| UNSAFE_className: cl( | ||
| item.props.UNSAFE_className, | ||
| stylesCommon.spectrumEllipsis | ||
| ), | ||
| }) | ||
| : item | ||
| ); |
There was a problem hiding this comment.
Ah, when we were talking about this, I didn't realize the children we were talking about were inside the picker items - i.e. there's no need for a key.
You should be able to get around this warning by just using React.Children instead:
content = React.Children.map(content, (element, i) =>
isElementOfType(element, Text)
? cloneElement(element, {
...element.props,
UNSAFE_className: cl(
element.props.UNSAFE_className,
stylesCommon.spectrumEllipsis
),
})
: element
);
There was a problem hiding this comment.
Ah nice, glad you knew about this one. It felt gross the way I implemented it.
|
Need to update e2e snapshots as well. |
| @@ -56,7 +56,7 @@ export function SamplesMenu(): JSX.Element { | |||
| ); | |||
|
|
|||
| const spectrumComparisonSamples = document.querySelector( | |||
| `#${SPECTRUM_COMPARISON_SAMPLES_ID}` | |||
| `#sample-section-${SPECTRUM_COMPARISON_SAMPLES_ID}` | |||
There was a problem hiding this comment.
This was a bug causing duplicate ids and the menu would always navigate to the first occurrence
textValuerightSupports deephaven/deephaven-plugins/issues/293