chore(card view demos): update to new selectable card variant #9322
Conversation
|
Preview: https://patternfly-react-pr-9322.surge.sh A11y report: https://patternfly-react-pr-9322-a11y.surge.sh |
| </> | ||
| ) | ||
| }} | ||
| selectableActions={{ isChecked: selectedItems.includes(product.id), selectableActionId: `selectable-actions-item-${product.id}`, selectableActionAriaLabelledby: `${'card-view-' + key}`, name: `check-${product.id}`, onChange: this.state.onCardClick }} |
There was a problem hiding this comment.
Rather than calling onCardClick, could we add an onChange method to call instead? Might be the same functionality, but just to have a more appropriate name.
Though a question for @andrew-ronaldson . Do we want to trigger the drawer opening upon "selecting" a Card in this demo? Or should we add another clickable action somewhere that would do that (making the card title a button, adding an "open drawer" button somewhere, etc)?
There was a problem hiding this comment.
Good points - will be working on a Primary Detail refactor (#9363) and plan to add this change to the TS conversion PR based on the TS component syntax.
I have some thoughts for your question as well... I think we would want to toggle the drawer when clicking a card, but it would be mutually exclusive from "selecting". I.e. when the checkbox is clicked, the drawer is not toggled, but if a user clicks anywhere else on the card, the drawer is toggled. That also aligns with the updated Card View behavior based on your feedback above. WDYT @thatblindgeye ?
There was a problem hiding this comment.
There would probably be some issues with making the whole card clickable to trigger opening a drawer. The main thing I'm thinking of is how we would allow clicking the card via keyboard, as we wouldn't be able to wrap the card in a button since there's already interactive content inside of it. We could probably add several things to the card wrapper to do it (tabindex, an onClick, some role, and some labeling that signifies the card can be clicked to open a drawer + possibly the drawer's current expanded state), but that's adding a lot of ARIA and other attributes when we may not need to.
Another might be if there's more tweaking that has to be done if the card includes links (having to avoid opening the drawer on selection, kebab toggle click, and link click), though I imagine any steps taken to avoid triggering the drawer action on selection/kebab click would easily work for something like a link/some other action button.
Considering all of that I feel like it'd be best to utilize a separate button somewhere in the card. Definitely something design should weigh in on, though.
There was a problem hiding this comment.
@andrew-ronaldson do you mind weighing in on this? According to the design guidelines of primary detail, there are no card actions on the primary detail cards, so it looks like the drawer would be toggled when the user clicks anywhere on the card. Would it make sense to simply remove the card actions / checkboxes from the cards in order to match the design guidelines?
There was a problem hiding this comment.
FWIW, this is the current interaction behavior in the Primary Detail Card View demo:
Primary.Detail.Card.View.ineraction.behavior.mov
The Drawer opens unexpectedly when the kebab and checkbox are clicked (via mouse or keyboard). With those issues fixed, there'd then be no way to open the drawer via keyboard (if the cards remained as the clickable+selectable variant)
There was a problem hiding this comment.
This is a fun one! I see the demos for the full-page Primary details also have nested clickable content.
Short-term we could remove these interactive elements from the demo to align with the design guidance.
Looking ahead the new cards have the title as the clickable element so we could let that trigger the drawer without interfering with the other clickable elements.
There was a problem hiding this comment.
Thanks @andrew-ronaldson this is a great idea - updated the primary detail demo to set the title to a clickable link which triggers the drawer
|
From a visual/interaction standpoint, this looks good to me - the only comment I have is that I'm not sure whether the intended design on the clickable+selectable card was for the card outline to darken when the checkbox is selected. I thought just the checkbox was meant to change states (?) That's what we have in our design guidelines images anyway. Referring to this: I could be wrong though - going to double check with Tina who worked on the designs !! |
|
Just double checked with her and she said the outline should be the same whether the card is checked or not! |
There was a problem hiding this comment.
Thank you for investigating @mmenestr !
This implementation of Card View matches the Card View design guidelines https://staging.patternfly.org/patterns/card-view/design-guidelines where the behavior and styling is a bit different from the typical standalone cards.
Here is an example of the styling as shown in the design guidelines, where the selected card has a blue border around it:
|
Yes! When the card is only selectable, it has a blue border (seen here also https://staging.patternfly.org/components/card/design-guidelines#selectable-cards) but when the card is both selectable AND clickable, it does not (seen here: https://staging.patternfly.org/components/card/design-guidelines#selectable-and-clickable-cards) ! I think it was done this way to distinguish between the card being clicked vs selected. If the card is checked, we don't want it to have an outline because it'll feel like it's also been selected even though it might not have been. |
|
That makes sense, thank you for clarifying @mmenestr ! I've updated the demo and the blue border no longer appears when the card is checked |
|
Changes look good @jenny-s51, thanks! |
There was a problem hiding this comment.
The demos are looking great, thank you for the updates @jenny-s51!! Just had a few quick comments below
test push
|
@srambach would you mind taking a look at this one? |
|
Just reviewed this and noticed the selected state of the selectable and clickable card still has a change in card outline. The intended design wasn't that - but maybe it's fine (?) @andrew-ronaldson what do you think? This was the design: and this is what's in the demo (see change in outline from selected to non selected) |
There was a problem hiding this comment.
@mmenestr Yes looks like this divergence from the design appears in both the core and react workspaces currently, and is therefore out of scope of this original issue.
It seems like either we should create standalone issues to update core / react to match the existing design, or update the design guidelines to match the current behavior.
@andrew-ronaldson which direction would you suggest we take with this?
|
I agree, the reimplementation of the clickable/selectable card put that border change on it. If design doesn't want it, it would be a core task to remove it. |
|
@mmenestr @jenny-s51 @srambach I don't think the border change is needed for the selected card so we can make a Core issue to update the card to match the design guidelines. |
|
Thank you for your awesome feedback @srambach @andrew-ronaldson @mmenestr ! Opened an issue in core to update the card styling. When you have a moment, would be great if you could all kindly take a final look at this PR and approve if everything else looks good. |
…nfly#9322) * update to new selectable card variant * add primary detail updates * PR feedback from Eric * update primary detail demo * fix a11y * PR feedback from Margot * update primary detail demo * PR feedback from Eric test push
What: Closes #9280