feat: add card pinning and keep pinned results at top#682
Conversation
Cloudflare Pages preview
|
conradarcturus
left a comment
conradarcturus
left a comment
There was a problem hiding this comment.
It functions well!
I left some comments but they aren't blocking -- I'll leave it to your judgment.
| const bPinned = bIndex !== -1; | ||
| // Pinned entities always come first, in the order they were pinned, so they | ||
| // reliably stay at the top (and on the first page) across refreshes. | ||
| if (aPinned && bPinned) return aIndex - bIndex; |
There was a problem hiding this comment.
oh nice, I didn't expect this but it makes sense to use the pin array to decide the order of the pins. Originally I had thought it would stay in sorted order amongst the pins, but this makes sense too.
| tabIndex={0} | ||
| > | ||
| {showPinButton && ( | ||
| <HoverableButton |
There was a problem hiding this comment.
This is a lot of markup, it would probably be best to move this to its own class CardPinButton
| <span | ||
| aria-label={isPinned ? 'Unpin from the page' : 'Pin to the page'} | ||
| onMouseEnter={() => setIsHoveringPin(true)} | ||
| onMouseLeave={() => setIsHoveringPin(false)} |
There was a problem hiding this comment.
We should probably all these as parameters to HoverableButton, but..
For this button it may be better to be completely different and actually not use HoverableButton. One advantage of the other hoverable buttons is that uses CSS hover to color the button. but since you hardcoded the background we cannot switch the background color to the usual hover state (blue bg white foreground). The background is nice because it gives a wider click area. I do agree that given its role when its not in a hover state it is better transparent.
So 1 options 1) change the background color & foreground color when isHovering or 2) move most of the styling to CSS. You already gave it a class name, you can also append a class when its pinned (eg. className={'CardPinButton' + (isPinned ? ' pinned' : '')}.
|
|
||
| const isPinned = pinned.includes(object.ID); | ||
| const togglePin = useCallback(() => { | ||
| updatePageParams({ |
There was a problem hiding this comment.
If the card moves it stays in the hover state which is odd (however if the card did not move then this is wrong).
Hmmm, I think we may want to control colors with CSS -- I mentioned later that you can either use isHoveringPin to make more fine-grained controls but I notice that this doesn't fix all the rendering bugs (cards stay hovered even when it moves away from the cursor because the mouseLeave event did not trigger).
I think the css :hover attribute is more accurate than what we are doing now.
| updatePageParams({ | |
| setIsHoveringPin(false); | |
| updatePageParams({ |
2 participants
Fixes #659
Summary: Adds card pinning from the card list and ensures pinned entities stay visible at the top of filtered results.
Changes
User experience
Logical changes
Screenshots