-
Notifications
You must be signed in to change notification settings - Fork 371
feat(Tile): deprecate #10821
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(Tile): deprecate #10821
Conversation
Preview: https://patternfly-react-pr-10821.surge.sh A11y report: https://patternfly-react-pr-10821-a11y.surge.sh |
@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? |
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.
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:
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?
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"?
|
cc @edonehoo @thatblindgeye Updated with feedback. Added an Added another example for multiselect tiles as well. |
packages/react-core/src/deprecated/components/Tile/examples/Tile.md
Outdated
Show resolved
Hide resolved
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.
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
packages/react-core/src/deprecated/components/Tile/examples/Tile.md
Outdated
Show resolved
Hide resolved
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.
Missing border should be fixed by the correct application of the screenreader class |
Thanks for the updates! I noticed there isn't the usual spacing between the + icon and the card title. |
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 |
The old tile examples use |
@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 |
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.
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'; |
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 have a global class .pf-v6-screen-reader
that you can use instead of importing the entire utility class stylesheet.
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.
yes to Michael's comment |
@andrew-ronaldson yes plz! |
Latest changes look great! Thanks! |
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.