-
Notifications
You must be signed in to change notification settings - Fork 45
fix(Table): add support for clickable rows (and clickable cards) #4335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Barsnes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a story and doc (both react and html) for this on www as well?
| const getDelegateTarget = ({ target: el }: Event) => { | ||
| const scope = el instanceof Element ? el.closest(CLICKDELEGATE) : null; | ||
| const target = scope?.querySelector(CLICKTARGET); | ||
| const skip = target && (el as Element).closest(SKIP); // Ignore if interactive | ||
|
|
||
| return ((!skip || skip === target) && target) || undefined; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some error reporting here if it finds multiple targets?
I assume a data-clickdelegate should only have one data-clicktarget
| const getDelegateTarget = ({ target: el }: Event) => { | |
| const scope = el instanceof Element ? el.closest(CLICKDELEGATE) : null; | |
| const target = scope?.querySelector(CLICKTARGET); | |
| const skip = target && (el as Element).closest(SKIP); // Ignore if interactive | |
| return ((!skip || skip === target) && target) || undefined; | |
| }; | |
| const getDelegateTarget = ({ target: el }: Event) => { | |
| const scope = el instanceof Element ? el.closest(CLICKDELEGATE) : null; | |
| const target = scope?.querySelector(CLICKTARGET); | |
| const skip = target && (el as Element).closest(SKIP); // Ignore if interactive | |
| if (target.length > 1) console.warn(`Multiple ${CLICKTARGET} found for element: ` + el.closest(CLICKDELEGATE)) | |
| return ((!skip || skip === target) && target) || undefined; | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right just saw the querySelector, but we should probably check for more, to make sure this is used correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm, I do not think we need this - in that case we need to do querySelectorAll which causes worse performance. If someone puts multiple targets, isn't it okay we just default to the first found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine now in the preview deployments.
We'll wait with merging this until then.
Since this can be used basically anywhere, should we consider adding this under "utilities"? Since it is not specific to table, but table needs styling.
We also need to add tests for this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll wait with merging this until then.
With this I meant wait for sideEffects to be handled in a different pr.. :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added under utilites now, but need the useClickDelegate for now to work around tree shaking, and since we for now only demonstrate in table, I have added the loading in table
"We'll wait with merging this until then." - what is "then"? :D
tests are definitely needed. Is it something you guys can help with? (a bit stuck with some Mattilsynet chores here :o)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add tests to this :)
Also added a second comment about "then" 😓
|
I think PR looks good now. We should however make sure side effects is set to |
So do you want to wait for #4367 before merging? |
yes :) |
Summary
Background:
data-clickdelegateso it makes more sense to use in combination with button and input (not only links) - aligning with Open UIs latest discussions (Link delegation to descendant openui/open-ui#1104 (comment)), even though they have not updated the explainer page itself:click-delegate-hoverand:click-delegate-activewith class names:.\:click-delegate-hoverand.\:click-delegate:activematching the convention from https://github.com/oddbird/popover-polyfillNote:
Question:
This requiresid=""for all connections. I think the W3C Open UI group should consider thedata-clicktarget(ordefaultlink) attribute they initially proposed, so we do not need lots of id generation. This is an very early proposal, so we do not know where they land. Do you guys want us to go for thedata-clicktargetattribute, or theiddirection?✅ With
data-clicktarget: ⬅️ Implemented this solution after feedback from @stianmorsund❌
Withid:Checks
pnpm changesetif relevant)