-
Notifications
You must be signed in to change notification settings - Fork 329
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
feat(promts): skeleton UI for promts #5726
Conversation
<div | ||
{...{ | ||
className: header.column.getCanSort() | ||
? "cursor-pointer" | ||
: "", | ||
onClick: header.column.getToggleSortingHandler(), | ||
style: { | ||
textAlign: header.column.columnDef.meta?.textAlign, | ||
}, | ||
}} | ||
> |
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.
Why not unstyled button? Makes it tab-navigable and more accessible
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.
good point - these come from the tanstack examples - which tend to be div soup. In general I agree with you and don't know the exact rationale for this. Let me change.
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 added an aria-role for this for now. Good point on the tab order stuff - I'm not 100% sure that would be desired here so I'm just advertising to screen readers for now. Would have to do a bit more testing on this to see if it makes more sense to actually have the focus move to the sort indicator itself.
<tr | ||
key={row.id} | ||
onClick={() => { | ||
navigate(`${row.original.id}`); | ||
}} | ||
> |
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.
same here, although it should likely be a full-height/full-width anchor or button nested inside the tr
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.
This one I have a rationale for - it's that I at least place an anchor tag on the name so that becomes the main "accessible" way to navigate. The click on the TR is just meant to solve the "I just want to click on the full row" but maintains the DOM structure needed.
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.
Gotcha, sounds good
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
* wip * WIP * WIP * add the prompt type * add initial listing * feat: temporary prompt * final changes * add aria role
resolves #5725
Adds rudimentary listing of
Prompts