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.

Sign up for GitHub

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

Jump to bottom

Allow sending action which is rendered next to tile title #283

Merged
merged 6 commits into from
Feb 28, 2024
Merged

Allow sending action which is rendered next to tile title #283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
baa2529
Allow sending action which is rendered next to tile title
karelhala Feb 16, 2024
4a1d1b9
Change action to predefined button and add an example
karelhala Feb 19, 2024
509c856
Fix failing A11 tests
karelhala Feb 20, 2024
57d2953
Use meta ID if present for card IDs
karelhala Feb 20, 2024
5d2d9ee
fix(ascii): Remove ascii doc examples.
dlabaj Feb 27, 2024
7956278
fix(lint): Fixed linting issue.
dlabaj Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/module/src/catalog/QuickStartTile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@ interface QuickStartTileProps {
status: QuickStartStatus;
isActive: boolean;
onClick?: () => void;
action?: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if should just allow a callback function instead and build the bookmark button ourself inside. I got some strange results passing in the incorrect type of button.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are not going to build it internally, we need documentation for how to use these props and this component. At the very least, we should add comments to each of these QuickStartTileProps. Eventually, those props can be used in the quickstarts docs on PF.org

I'm noticing that there is no example in the PF docs for how to build a CustomCatalog as is done in the demo app. ideally, we'd have an example that would also expose the APIs for all these components consumers could be using. That would need to be done in a follow up issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this approach! I've changed it to be a button with predefined config. For power users I've also allowed passing buttonProps should they need to tinker with the button (for example changing the hover state etc.).

@nicolethoen good point on adding this to a CustomCatalog, I've added another section with same content as is rest and added the bookmark toggle.

}

const QuickStartTile: React.FC<QuickStartTileProps> = ({
quickStart,
status,
isActive,
onClick = () => {},
action
karelhala marked this conversation as resolved.
Show resolved Hide resolved
}) => {
const {
metadata: { name: id },
Expand Down Expand Up @@ -92,6 +94,7 @@ const QuickStartTile: React.FC<QuickStartTileProps> = ({
duration={durationMinutes}
type={type}
quickStartId={id}
action={action}
/>
}
onClick={handleClick}
Expand Down
5 changes: 5 additions & 0 deletions packages/module/src/catalog/QuickStartTileHeader.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,9 @@
& .pf-v5-c-badge:not(:last-of-type) {
margin-right: var(--pf-v5-global--spacer--sm);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm so conditioned to avoid writing custom CSS.

So my instinct here is to say that I think you could replace this div.pfext-quick-start-title-header__display-name with :

<Flex  justifyContent={{ default: 'justifyContentCenter' }}>
       <Title headingLevel="h3" data-test="title" id={quickStartId}>
          <QuickStartMarkdownView content={name} />
        </Title>
        {action}
</Flex>

And you might opt to wrap the children in a FlexItem but i don't think that'll be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree. i'm just use to custom css breaking i try to avoid it.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm still concerned about the action though. since i think you can't just really put anything there and someone may try to do that.

.pfext-quick-start-title-header__display-name {
display: flex;
justify-content: center;
}
}
11 changes: 8 additions & 3 deletions packages/module/src/catalog/QuickStartTileHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ interface QuickStartTileHeaderProps {
name: string;
type?: QuickStartType;
quickStartId?: string;
action?: React.ReactNode;
}

const statusColorMap = {
Expand All @@ -27,6 +28,7 @@ const QuickStartTileHeader: React.FC<QuickStartTileHeaderProps> = ({
name,
type,
quickStartId,
action
karelhala marked this conversation as resolved.
Show resolved Hide resolved
}) => {
const { getResource } = React.useContext<QuickStartContextValues>(QuickStartContext);

Expand All @@ -38,9 +40,12 @@ const QuickStartTileHeader: React.FC<QuickStartTileHeaderProps> = ({

return (
<div className="pfext-quick-start-tile-header">
<Title headingLevel="h3" data-test="title" id={quickStartId}>
<QuickStartMarkdownView content={name} />
</Title>
<div className="pfext-quick-start-title-header__display-name">
<Title headingLevel="h3" data-test="title" id={quickStartId}>
<QuickStartMarkdownView content={name} />
</Title>
{action}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's your thoughts on creating the bookmark internally? I was able to create some strange results passing in the button component not set to a link. The alignment also looked good in some cases but if the title got to large it looked to low. I would think we would want it to stick to the top. Let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it from the start as well, but I was not that big of a fan of it. Good point on style misalignment, how about we change this to an object with actionIcon, onActionClick and actionAriaLabel. Right now we want to show plain button. If someone would want to display a dropdown in here, with these 3 items they should be able to do that (by using dropdown menu and attaching it to the right place) should there be such need in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me. Maybe make one of the cards in the demo doc with how to implement it. So people would know.

</div>
<div className="pfext-quick-start-tile-header__status">
{type && (
<Label className="pfext-quick-start-tile-header--margin" color={type.color}>
Expand Down
Loading