fix: allow users to choose properties like data block table - #1914
webmagic123 wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
… property menu icon based on edit mode status
6d41976 to
f06dcc6
Compare
b-d055
left a comment
b-d055
left a comment
There was a problem hiding this comment.
Really like the direction here. A few small suggestions inline, nothing blocking. Please also resolve conflicts with master
| const entityIdsKey = entityIds.filter(Boolean).join('|'); | ||
| const stableIds = React.useMemo(() => [...new Set(entityIdsKey ? entityIdsKey.split('|') : [])], [entityIdsKey]); | ||
|
|
||
| useHydrateEntities({ ids: stableIds, spaceId, enabled: stableIds.length > 0 }); |
There was a problem hiding this comment.
useQueryEntities right below already hydrates these with their relations, and since this passes spaceId while the query passes undefined, they end up keyed separately and fire as two fetches. This hook runs a few times per screen, so if the goal is just warming the space scoped values, it might be worth seeing whether feeding spaceId into the query itself could cover both and drop the extra request.
| }; | ||
|
|
||
| export function selectRankingCardProperties(properties: Property[]): Property[] { | ||
| return properties.filter(property => !RANKING_CARD_EXCLUDED_PROPERTY_IDS.has(property.id)); |
There was a problem hiding this comment.
Small inconsistency: this uses an exact Set.has, but selectRankingCardImageProperty just below compares the same Cover id with ID.equals. These column ids come through dashed in some places, so if that ever happens here Cover could slip past the filter and render as both the card image and a duplicate field. Might be worth normalizing so the two selectors agree.
|
|
||
| type Props = { | ||
| /** Omit or pass 0 to hide the rank indicator. */ | ||
| rank?: number; |
There was a problem hiding this comment.
These prop docs got dropped in the reshuffle. They were handy context, especially the leading vs avatar-badge one, if you want to keep them.
|
|
||
| return ( | ||
| <div className="flex w-full min-w-0 items-center gap-4 overflow-hidden"> | ||
| <div className="flex w-full min-w-0 items-start gap-4 overflow-hidden"> |
There was a problem hiding this comment.
Switching to items-start (and dropping justify-center below) reads well when properties are stacked underneath, but for a name only card it top aligns the name against the avatar instead of centering it. Worth a quick visual check since this touches every card, not just the new ones.
| {cardProperties.length > 0 ? ( | ||
| <div className="flex flex-col gap-1"> | ||
| {cardProperties.map(property => ( | ||
| <TableBlockPropertyField |
There was a problem hiding this comment.
Heads up that a boolean property renders as a live checkbox here even for viewers who aren't editing, since the checkbox in this component isn't gated by edit mode. Existing behavior, just newly visible on cards, so worth confirming that's intended for a display surface.
| const createNewSpaceId = React.useMemo(() => resolveRankingSingleTargetSpaceId(filterState), [filterState]); | ||
|
|
||
| const cardImageProperty = React.useMemo(() => selectRankingCardImageProperty(shownColumnIds), [shownColumnIds]); | ||
| const cardConfig = React.useMemo( |
There was a problem hiding this comment.
This rebuilds the same card config inline instead of reusing useRankingShownProperties. Works fine, but it could drift from the hook over time if one changes.
…in-the-ranking-cards
|
Closing as part of a sweep of the open-PR queue — not a judgement on the work, and please reopen if you still want it. Opened 2026-06-18, and it now conflicts with master. At this distance a rebase is usually more work than redoing the change against current code, and the property picker has almost certainly moved underneath it. @webmagic123 — if this is still wanted, reopen it and I will help get it current. If the idea still stands but the branch does not, it is probably worth a fresh PR or a ticket rather than reviving this one. Context: there are 60 open PRs, 10 older than two months. The intent is to make the queue mean something, so that genuinely ready work is visible instead of buried — #2449 sat ready for three days this week partly for that reason. Nothing here is being discarded: the branch and its history stay, and reopening costs a click. |
This branch was successfully deployed
3 participants
No description provided.