Skip to content
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(Card) implement new selectable/clickable card design #9092

Merged
merged 13 commits into from May 18, 2023

Conversation

Dominik-Petrik
Copy link
Contributor

What: Closes #8685

@Dominik-Petrik Dominik-Petrik marked this pull request as draft May 9, 2023 16:55
@patternfly-build
Copy link
Contributor

patternfly-build commented May 9, 2023

@thatblindgeye thatblindgeye force-pushed the cardAffordances branch 2 times, most recently from da7cc60 to 0bd78c0 Compare May 10, 2023 19:32
@thatblindgeye
Copy link
Contributor

Clickable card example (plus the Selectable, Single selectable, and "Clickable and selectable" examples below it)

Some notes:

  • The idea of having both a selectableActions and clickableActions prop was tossed around. Ultimately we went with a single prop, seeing everything come together it may make sense to go with the two prop route
  • Due to needing a specific markup for the styling to be correct, we're rendering an input internally for clickable or selectable cards rather than relying on it being passed in manually.
  • Tests have yet to be written; I wanted to get feedback on the API first. If we're tight on time I can open a followup to write out the tests cc @tlabaj
  • hasNoOffset can be passed to selectableActions because there would need to be a way to apply the class to the wrapping CardActions element. An alternative would be to make the actions prop on CardHeaderActionsObject no longer required, so that actions={{hasNoOffset: true}} could be passed
  • Updated old examples to be titled "Legacy" and deprecated the applicable props
  • I had to remove internally from Card the tabIndex being applied based on isSelectable being passed, and instead updated examples to manually pass the tabIndex in

@thatblindgeye thatblindgeye marked this pull request as ready for review May 10, 2023 19:57
@mcarrano mcarrano requested a review from mmenestr May 10, 2023 20:28
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thatblindgeye this looks great! I'd like to get a couple more designers to looks at this before clicking the Approve button, but all looks good to be.

@mmenestr and @tiyiprh can you also take a look? Note that in this example (https://patternfly-react-pr-9092.surge.sh/components/card/#clickable-and-selectable-cards) we are allowing for the card action to be placed on the title as in the designs or another element on the card. We felt like it made sense to allow this flexibility even if the default design recommendation will be to place the card action on the title element.

@tiyiprh
Copy link

tiyiprh commented May 10, 2023

This looks great! I think the example of the clickable & selectable cards make sense. I think this would also take care of the use case of the card action being placed in the footer to an external link, correct?

@thatblindgeye
Copy link
Contributor

@tiyiprh Yep! We're basically just showing how to make a selectable card also contain some other interactive action, and just using the CardTitle or a link in the CardBody as an example. It could also be used to show how you'd now place a kebab toggle inside a selectable card as well (quickly thrown together):

New selectable card with clickable kebab

@mmenestr
Copy link
Collaborator

This looks great!! Just have 2 nitpicks:

  1. For the first clickable card, I'm not able to un-click it. Like when I click on it, it gets highlighted, but then when I click on it again, it doesn't get unhighlighted like I would expect it to!

  2. In the second clickable and selectable card, there's a space between PatternFly and the period at the end of the sentence

@thatblindgeye
Copy link
Contributor

For the first clickable card, I'm not able to un-click it. Like when I click on it, it gets highlighted, but then when I click on it again, it doesn't get unhighlighted like I would expect it to!

Ah yeah this might be a limitation to using a radio input under the hood...

@srambach It may work out better to use a button/anchor under the hood for clickable cards despite some of the issues with that (we can hopefully mitigate those by our documentation recommending not using clickable only cards if the card should contain other interactive content?). I threw something together to try and offer some "unclick-ability" by basically hiding another radio so that when you click a checked radio, it sets that hidden radio to check instead (only did this in one of our Radio component examples). I wasn't able to unclick with keyboard though. I could try updating the styling and examples in Core first thing next week if you think it's worth a shot.

@mcarrano
Copy link
Member

For the first clickable card, I'm not able to un-click it. Like when I click on it, it gets highlighted, but then when I click on it again, it doesn't get unhighlighted like I would expect it to!

@mmenestr I understand what you are saying but I wonder if this is really a problem given the use cases this is intended to address. If the clickable card is being used for navigation or to select an object to view details in a primary-detail layout, would you really need to de-select it? Seems like in the primary-detail view, an object would remain selected until a different object is clicked on, so I would expect that clicking a selected card has no effect. @tiyiprh what are your thoughts?

@tiyiprh
Copy link

tiyiprh commented May 15, 2023

For the first clickable card, I'm not able to un-click it. Like when I click on it, it gets highlighted, but then when I click on it again, it doesn't get unhighlighted like I would expect it to!

@mmenestr I understand what you are saying but I wonder if this is really a problem given the use cases this is intended to address. If the clickable card is being used for navigation or to select an object to view details in a primary-detail layout, would you really need to de-select it? Seems like in the primary-detail view, an object would remain selected until a different object is clicked on, so I would expect that clicking a selected card has no effect. @tiyiprh what are your thoughts?

At first I thought it might be helpful to be able to un-click it as well but I also see what you're saying in that it might not be necessary. On the current primary-detail experience on Patternfly, it looks like the data lists do not have an un-click action but the card view does (not sure if there is a bug on the card view?). I feel like being able to un-click it would be a nice to have in that the user can have another way to close the primary-detail view but if the current experience does not allow for it and there hasn't been any problems with that, maybe it's okay to keep the same experience? Especially if it seems like it's not a simple fix.

@mmenestr
Copy link
Collaborator

@mcarrano it seems like both are possible. In the case where a drawer opens in a way that overlays the contents, and doesn't just push it aside, you might want to have the ability to close the drawer (by unselecting the card) in order to see all the options clearly. Though they could arguably X out of it via the drawer, I think clicking on the card to open and close is more intuitive.

@mcarrano
Copy link
Member

In the case where a drawer opens in a way that overlays the contents, and doesn't just push it aside, you might want to have the ability to close the drawer (by unselecting the card) in order to see all the options clearly.

So in this case the card works more like a toggle? I can see that as a nice-to-have option. @thatblindgeye do you feel like that would be possible without too much rework? If not, then I'd propose to stick with the current behavior and we can open a request to add this as a future enhancement.

@srambach
Copy link
Member

If being able to have no card selected after selecting one, can you progammatically un-select the card (radio button)? That seems the best option to me. A checkbox would allow multiples and the button has the problem of having actionable items inside.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good a few comments.

@thatblindgeye
Copy link
Contributor

@mcarrano @srambach I've been able to get close to making a clickable card "unclickable", but there's still some gotchas with it (mainly that we have to call the onActionClick on both click and keydown, which causes it to fire twice). Checkboxes might be easier in being able to unclick something, though from an AT point of view yeah it'll make it seem like you can click multiple cards at once.

Using buttons/anchors would require a markup and style change in Core, but for that "unclick"-ability it might be the better option if we could avoid having interactive content inside those elements (or style them in a way where nothing actually gets placed inside them).

Either way it might work better as a possible enhancement in the future.

}}
/>
<CardTitle>Second card</CardTitle>
<CardBody>This card can navigate to a link on click.`.</CardBody>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an extra `. here

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@srambach
Copy link
Member

These new examples have their title outside the card__header. Can it be placed inside so that the alignment is better? Core has:

<div class="pf-v5-c-card pf-m-clickable" id="card-clickable-example">
   <div class="pf-v5-c-card__header">
     <div class="pf-v5-c-card__actions pf-m-no-offset">
{snip}
     </div>
     <div class="pf-v5-c-card__header-main">
       <div class="pf-v5-c-card__title" id="card-clickable-example-title">
         <h2 class="pf-v5-c-card__title-text">Title</h2>
       </div>
     </div>
   </div>
   <div class="pf-v5-c-card__body">Body</div>
   <div class="pf-v5-c-card__footer">Footer</div>
 </div>
image

@thatblindgeye
Copy link
Contributor

@srambach ah good catch, updated the examples!

@srambach
Copy link
Member

Nit - but could these new examples be in the same order as the core examples? It makes it easier to understand how they relate.

@thatblindgeye
Copy link
Contributor

@srambach reordered + renamed to drop the repetitive "cards" at the end of th example names

Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the updates 👍

@tlabaj tlabaj merged commit c3cf823 into patternfly:v5 May 18, 2023
10 checks passed
jelly added a commit to jelly/cockpit-podman that referenced this pull request Jul 25, 2023
jelly added a commit to jelly/cockpit-podman that referenced this pull request Jul 26, 2023
jelly added a commit to jelly/cockpit-podman that referenced this pull request Jul 26, 2023
jelly added a commit to jelly/cockpit-podman that referenced this pull request Jul 26, 2023
martinpitt pushed a commit to cockpit-project/cockpit-podman that referenced this pull request Jul 26, 2023
nicolethoen pushed a commit to Kells512/patternfly-react that referenced this pull request Sep 1, 2023
…#9092)

* feat(Card) implement new selectable/clickable card design

* selectable and clickable changes

* add selectableVariant, refactor markup

* add clickable & selectable

* Updated functionality

* update examples, add functionality

* Update selectableActions api

* Added clickable and selectable example

* Updated tests

* Updated legacy verbiage and logic to render selectable cards

* Added prop for link target

* Updated example markup

* Updated example order

---------

Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Card - implement new selectable/clickable card design