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

karelhala · 2024-02-16T14:41:43Z

Description

In our quickstarts we need to render a bookmark button that bookmarks the quickstart. In order to render said button we need to send a button with this icon to the quickstart. This PR surfaces this trough quickStartTile to the header.

Screenshot

netlify · 2024-02-16T14:42:22Z

✅ Deploy Preview for quickstarts ready!

Name	Link
🔨 Latest commit	`7956278`
🔍 Latest deploy log	https://app.netlify.com/sites/quickstarts/deploys/65de0ac3e418d00008c95d19
😎 Deploy Preview	https://deploy-preview-283--quickstarts.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dlabaj

Overall looks good. Just wondering if we would be better off building the bookmark button internally instead of allowing some to make it themself. There where some strange layout issues if they didn't build it correctly. Let me know.

dlabaj · 2024-02-16T16:31:38Z

packages/module/src/catalog/QuickStartTileHeader.tsx

+        <Title headingLevel="h3" data-test="title" id={quickStartId}>
+          <QuickStartMarkdownView content={name} />
+        </Title>
+        {action}


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.

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.

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

dlabaj · 2024-02-16T16:33:20Z

packages/module/src/catalog/QuickStartTile.tsx

@@ -17,13 +17,15 @@ interface QuickStartTileProps {
  status: QuickStartStatus;
  isActive: boolean;
  onClick?: () => void;
+  action?: React.ReactNode;


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.

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.

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.

packages/module/src/catalog/QuickStartTileHeader.tsx

packages/module/src/catalog/QuickStartTile.tsx

nicolethoen · 2024-02-16T19:09:12Z

packages/module/src/catalog/QuickStartTile.tsx

@@ -17,13 +17,15 @@ interface QuickStartTileProps {
  status: QuickStartStatus;
  isActive: boolean;
  onClick?: () => void;
+  action?: React.ReactNode;


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.

nicolethoen · 2024-02-16T19:17:50Z

packages/module/src/catalog/QuickStartTileHeader.scss

@@ -9,4 +9,9 @@
  & .pf-v5-c-badge:not(:last-of-type) {
    margin-right: var(--pf-v5-global--spacer--sm);
  }
+


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.

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

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.

packages/dev/src/CustomCatalog.tsx

dlabaj

LGTM with the new changes. Seems to be failing an A11y test if you need help with that let me know.

karelhala · 2024-02-20T16:34:08Z

@dlabaj thank you! I might need some help with the duplicate IDs, they look like are not par of my changes. However I had to change how ID is generated for tile, it used to take just name. Now it consumes ID as well and if that is present it uses this ID. I then changed how quickstart is activated and use just the name of quickstart (as ID can be different than name).

nicolethoen · 2024-02-26T16:30:34Z

Okay - i've discovered that the duplicate id in these docs is description-alert-note
I'm about to try and fix it, but if knowing that means someone knows the quick fix, then let me know.

nicolethoen · 2024-02-26T21:26:56Z

CustomCatalog is looping over the same list twice of allQuickStarts to build the Bookmarkable and the Instructional lists of Quickstart tiles.

the hardcoded id on line 8 of alert-note-prereq/README.adoc is what is being used twice in the docs

karelhala · 2024-02-27T14:48:02Z

@nicolethoen yeah I wanted to duplicate the quickstarts as an example of bookmarked items. I am starting to think about removing the custom catalog changes and let PF team to do proper examples 😞

dlabaj · 2024-02-27T16:18:17Z

@karelhala We decided to remove the asciidoc examples. The only product that uses it was kafka but that product is no longer available. While the feature to use the asciidoc is still there we are going to encourage people to use markdown instead.

dlabaj · 2024-02-27T21:44:38Z

This issue is causing the Percy test to fail. patternfly/patternfly#6352. We will need a fix from core. It only affects chrome and works in firefox. I wouldn't hold up this merge to quickstart over the failing Percy test. Let me know what you think @jessiehuff @nicolethoen

dlabaj

LGTM Tested it in firefox and it works.

github-actions · 2024-03-07T12:08:50Z

🎉 This PR is included in version 5.2.0-prerelease.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

…#283) * Allow sending action which is rendered next to tile title * Change action to predefined button and add an example * Fix failing A11 tests * Use meta ID if present for card IDs * fix(ascii): Remove ascii doc examples. * fix(lint): Fixed linting issue. --------- Co-authored-by: Donald Labaj <[email protected]>

Allow sending action which is rendered next to tile title

baa2529

dlabaj requested review from dlabaj, jessiehuff, jschuler and nicolethoen February 16, 2024 15:55

dlabaj reviewed Feb 16, 2024

View reviewed changes

packages/module/src/catalog/QuickStartTileHeader.tsx Outdated Show resolved Hide resolved

dlabaj reviewed Feb 16, 2024

View reviewed changes

packages/module/src/catalog/QuickStartTile.tsx Outdated Show resolved Hide resolved

nicolethoen reviewed Feb 16, 2024

View reviewed changes

karelhala force-pushed the add-tile-action branch 3 times, most recently from f0fce4f to 30e83ec Compare February 19, 2024 08:22

karelhala commented Feb 19, 2024

View reviewed changes

packages/dev/src/CustomCatalog.tsx Show resolved Hide resolved

karelhala force-pushed the add-tile-action branch from 30e83ec to 8c8bae6 Compare February 19, 2024 09:14

karelhala mentioned this pull request Feb 19, 2024

Add bookmarks to learning resources RedHatInsights/learning-resources#43

Merged

karelhala force-pushed the add-tile-action branch 2 times, most recently from ba865f6 to ec214d9 Compare February 19, 2024 09:30

Change action to predefined button and add an example

4a1d1b9

karelhala force-pushed the add-tile-action branch from ec214d9 to 4a1d1b9 Compare February 19, 2024 10:57

dlabaj approved these changes Feb 20, 2024

View reviewed changes

Fix failing A11 tests

509c856

Use meta ID if present for card IDs

57d2953

karelhala force-pushed the add-tile-action branch from 7a4f46f to 57d2953 Compare February 21, 2024 08:46

dlabaj added 2 commits February 27, 2024 11:01

fix(ascii): Remove ascii doc examples.

5d2d9ee

fix(lint): Fixed linting issue.

7956278

dlabaj approved these changes Feb 27, 2024

View reviewed changes

dlabaj merged commit 0babc9c into patternfly:main Feb 28, 2024
9 of 10 checks passed

github-actions bot added the released on @prerelease label Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

karelhala commented Feb 16, 2024 •

edited

Loading

netlify bot commented Feb 16, 2024 •

edited

Loading

dlabaj left a comment

dlabaj Feb 16, 2024

karelhala Feb 19, 2024

dlabaj Feb 20, 2024

dlabaj Feb 16, 2024

nicolethoen Feb 16, 2024

karelhala Feb 19, 2024

nicolethoen Feb 16, 2024

nicolethoen Feb 16, 2024

dlabaj Feb 16, 2024

dlabaj Feb 16, 2024

dlabaj left a comment

karelhala commented Feb 20, 2024

nicolethoen commented Feb 26, 2024

nicolethoen commented Feb 26, 2024

karelhala commented Feb 27, 2024

dlabaj commented Feb 27, 2024

dlabaj commented Feb 27, 2024

dlabaj left a comment

github-actions bot commented Mar 7, 2024

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

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

Conversation

karelhala commented Feb 16, 2024 • edited Loading

Description

Screenshot

netlify bot commented Feb 16, 2024 • edited Loading

✅ Deploy Preview for quickstarts ready!

dlabaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlabaj left a comment

Choose a reason for hiding this comment

karelhala commented Feb 20, 2024

nicolethoen commented Feb 26, 2024

nicolethoen commented Feb 26, 2024

karelhala commented Feb 27, 2024

dlabaj commented Feb 27, 2024

dlabaj commented Feb 27, 2024

dlabaj left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2024

karelhala commented Feb 16, 2024 •

edited

Loading

netlify bot commented Feb 16, 2024 •

edited

Loading