Skip to content

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Jul 30, 2024

What: Closes #8542

Moves Tile to deprecated directory.
Moves Tile demos to Tile examples so they are all sorted under a deprecated tab.
Adds a new Card demo meant to mock up Tile - may need some additional work/support to fully replace Tile.

@kmcfaul kmcfaul linked an issue Jul 30, 2024 that may be closed by this pull request
@patternfly-build
Copy link
Contributor

patternfly-build commented Jul 30, 2024

@kmcfaul kmcfaul marked this pull request as draft July 31, 2024 14:01
@kmcfaul kmcfaul marked this pull request as ready for review August 5, 2024 19:52
@kmcfaul
Copy link
Contributor Author

kmcfaul commented Aug 8, 2024

@patternfly/design-reviewers Re: the new example in Card, is that sufficient for "Card as a tile" (since Cards kind of replace Tile, but don't look exactly the same), or do we want to revisit tile design and maybe make updates to Card?

@kmcfaul kmcfaul changed the title feat(Tile): deprecate in docs feat(Tile): deprecate Aug 19, 2024
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Regarding the comment above, maybe we should introduce that prop we originally were going to to visually hide the input element? Like the following screenshot:

A card component acting as a tile, with the radio input visually hidden

Could we also add an example description? Maybe mentioning that a card as a tile can be setup with either single or multi selectability and, if we add a prop to hide the input, mentioning that. cc @edonehoo do you have any suggestions?

@edonehoo
Copy link
Contributor

I think an example description would be helpful. Are these only selectable as a toggle function, rather than triggering an action or an outbound link?

"Card tiles"?
And maybe adding checkboxes/radios within the example to toggle between:

  • single select input
  • multi select input
  • no input

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Aug 27, 2024

cc @edonehoo @thatblindgeye Updated with feedback.

Added an isHidden property to the selectableActions object, though fwiw it technically would already be possible to hide the radio/checkbox on the user end if they passed selectableActionProps: { style: { visibility: 'hidden' } } to selectableActions. isHidden is just a shorthand for setting the style manually, and we could remove it and update the examples if we'd prefer to leave it out of the props.

Added another example for multiselect tiles as well.

@tlabaj tlabaj requested review from a team, andrew-ronaldson, mattnolting, mmenestr, tlabaj and wise-king-sullyman and removed request for a team and mmenestr August 28, 2024 14:38
Copy link
Contributor

@edonehoo edonehoo left a comment

Choose a reason for hiding this comment

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

A few suggestions!

Just fyi (and a note to myself), will also need to move some of the tile guidelines in the card design guidelines for more info here, so I'll handle that

Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

Just noticed the examples don't line up. The border is missing on the card as a tile section
Tile page
Screenshot 2024-09-23 at 12 11 03 PM

Card as tile page
Screenshot 2024-09-23 at 12 11 30 PM

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Sep 23, 2024

Missing border should be fixed by the correct application of the screenreader class

@andrew-ronaldson
Copy link
Collaborator

andrew-ronaldson commented Sep 24, 2024

Thanks for the updates! I noticed there isn't the usual spacing between the + icon and the card title.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Sep 25, 2024

usual spacing

Might be overlooking it, but Card doesn't have a built in support for icon with text like via a prop (the Card example with an icon and text uses Brand) so I've passed it directly. I did increase the spacing by wrapping them with a Flex. How does that look? @andrew-ronaldson

@andrew-ronaldson
Copy link
Collaborator

The old tile examples use margin-inline-end: var(--pf-v6-c-tile__icon--MarginInlineEnd); for the spacer after the icon.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Sep 25, 2024

@andrew-ronaldson @mcoker Card doesn't have an equivalent for icon spacing in the header. Could this be added post-v6 in a follow up if the Flex spacing is sufficient for now? Or should I add manual styling that uses the deprecated tile css?

@mcoker
Copy link
Contributor

mcoker commented Sep 25, 2024

@andrew-ronaldson @mcoker Card doesn't have an equivalent for icon spacing in the header. Could this be added post-v6 in a follow up if the Flex spacing is sufficient for now? Or should I add manual styling that uses the deprecated tile css?

@kmcfaul (as discussed in standup) IMO we should add support for an icon in the card header, but I think that can be a follow-up since it would be non-breaking to add the enhancement then update our example. Using <Flex> lgtm for now

Copy link
Contributor

@mcoker mcoker 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 to me though if possible...

  • I left a comment about using .pf-v6-screen-reader instead of importing the a11y utility class. Sometimes those utility class stylesheets can be really big, so the standalone class would be preferred.
  • For the flex layout with the icon/text, I'd bump the spacer down to 'sm', and use the gap/column-gap spacer (instead of the default spacer system, which uses margins). The core class to add would be .pf-m-gap-sm. You could also add whatever the react prop is for .pf-m-align-items-center and it will vertically align the icon and text (as long as the text doesn't wrap)

import * as React from 'react';
import { css } from '@patternfly/react-styles';
import styles from '@patternfly/react-styles/css/components/Card/card';
import accessibilityStyles from '@patternfly/react-styles/css/utilities/Accessibility/accessibility';
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a global class .pf-v6-screen-reader that you can use instead of importing the entire utility class stylesheet.

Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

Sounds good @kmcfaul and @mcoker. I agree with adding card with icon in header support. Should I make a Core issue?

@andrew-ronaldson
Copy link
Collaborator

  • For the flex layout with the icon/text, I'd bump the spacer down to 'sm', and use the gap/column-gap spacer (instead of the default spacer system, which uses margins). The core class to add would be .pf-m-gap-sm. You could also add whatever the react prop is for .pf-m-align-items-center and it will vertically align the icon and text (as long as the text doesn't wrap)

yes to Michael's comment

@mcoker
Copy link
Contributor

mcoker commented Sep 25, 2024

Should I make a Core issue?

@andrew-ronaldson yes plz!

@andrew-ronaldson
Copy link
Collaborator

Latest changes look great! Thanks!

@tlabaj tlabaj dismissed edonehoo’s stale review September 26, 2024 13:42

Comments have been addressed.

@tlabaj tlabaj merged commit 548cd34 into patternfly:main Sep 26, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tile - deprecate Tile component

8 participants