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

docs(card): explain element vs pattern #1188

Merged
merged 27 commits into from
Aug 6, 2024
Merged

Conversation

bennypowers
Copy link
Member

What I did

  1. added an explanation on the card pattern page about rh-card element vs card patterns

Notes to Reviewers

Design should review the approach taken here and comment. It seems to me that this page could use some prose for each pattern usage guidelines etc)

@changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2023

⚠️ No Changeset found

Latest commit: c74498c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Aug 6, 2023

Deploy Preview for red-hat-design-system ready!

Name Link
🔨 Latest commit c74498c
🔍 Latest deploy log https://app.netlify.com/sites/red-hat-design-system/deploys/66b22f853765a300084bd931
😎 Deploy Preview https://deploy-preview-1188--red-hat-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

Size Change: 0 B

Total Size: 250 kB

ℹ️ View Unchanged
Filename Size
./elements/rh-accordion/context.js 162 B
./elements/rh-accordion/rh-accordion-header.js 3.74 kB
./elements/rh-accordion/rh-accordion-panel.js 1.47 kB
./elements/rh-accordion/rh-accordion.js 3.7 kB
./elements/rh-alert/rh-alert.js 4.49 kB
./elements/rh-audio-player/rh-audio-player-about.js 1.68 kB
./elements/rh-audio-player/rh-audio-player-button.css.js 437 B
./elements/rh-audio-player/rh-audio-player-panel.css.js 554 B
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.51 kB
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.01 kB
./elements/rh-audio-player/rh-audio-player.js 14.4 kB
./elements/rh-audio-player/rh-cue.js 2.01 kB
./elements/rh-audio-player/rh-transcript.js 2.29 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.93 kB
./elements/rh-back-to-top/rh-back-to-top.js 2.1 kB
./elements/rh-badge/rh-badge.js 1.04 kB
./elements/rh-blockquote/rh-blockquote.js 1.96 kB
./elements/rh-button/rh-button.js 4.45 kB
./elements/rh-card/rh-card.js 1.96 kB
./elements/rh-code-block/rh-code-block.js 5.62 kB
./elements/rh-cta/rh-cta.js 4.59 kB
./elements/rh-dialog/rh-dialog.js 4.79 kB
./elements/rh-dialog/yt-api.js 617 B
./elements/rh-footer/rh-footer-block.js 770 B
./elements/rh-footer/rh-footer-copyright.js 362 B
./elements/rh-footer/rh-footer-links.js 1.17 kB
./elements/rh-footer/rh-footer-social-link.js 1.35 kB
./elements/rh-footer/rh-footer-universal.js 2.05 kB
./elements/rh-footer/rh-footer.css.js 2.16 kB
./elements/rh-footer/rh-footer.js 3.04 kB
./elements/rh-footer/rh-global-footer.js 250 B
./elements/rh-menu/rh-menu.js 1.18 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.58 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 1.46 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu.js 1.9 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-overlay.js 571 B
./elements/rh-navigation-secondary/rh-navigation-secondary.js 5.55 kB
./elements/rh-navigation-secondary/test/fixtures.js 846 B
./elements/rh-pagination/rh-pagination.js 4.47 kB
./elements/rh-site-status/rh-site-status.js 3.18 kB
./elements/rh-skip-link/rh-skip-link.js 1.13 kB
./elements/rh-spinner/rh-spinner.js 1.61 kB
./elements/rh-stat/rh-stat.js 2.24 kB
./elements/rh-subnav/rh-subnav.js 2.83 kB
./elements/rh-surface/rh-surface.js 868 B
./elements/rh-table/rh-sort-button.js 1.49 kB
./elements/rh-table/rh-table.js 3.02 kB
./elements/rh-tabs/context.js 160 B
./elements/rh-tabs/rh-tab-panel.js 1.15 kB
./elements/rh-tabs/rh-tab.js 2.92 kB
./elements/rh-tabs/rh-tabs.js 4.05 kB
./elements/rh-tag/rh-tag.js 2.06 kB
./elements/rh-tile/rh-tile-group.js 1.76 kB
./elements/rh-tile/rh-tile.js 4.85 kB
./elements/rh-timestamp/rh-timestamp.js 983 B
./elements/rh-tooltip/rh-tooltip.js 2.24 kB
./lib/context/color/consumer.js 1.16 kB
./lib/context/color/context-color.css.js 268 B
./lib/context/color/controller.js 885 B
./lib/context/color/provider.js 2.04 kB
./lib/context/event.js 583 B
./lib/context/headings/consumer.js 722 B
./lib/context/headings/controller.js 1.12 kB
./lib/context/headings/provider.js 1.24 kB
./lib/DirController.js 565 B
./lib/elements/rh-context-demo/rh-context-demo.js 1.15 kB
./lib/elements/rh-context-picker/rh-context-picker.js 2.23 kB
./lib/functions.js 175 B
./lib/I18nController.js 1.38 kB
./lib/ScreenSizeController.js 849 B
./react/rh-accordion/rh-accordion-header.js 215 B
./react/rh-accordion/rh-accordion-panel.js 185 B
./react/rh-accordion/rh-accordion.js 215 B
./react/rh-alert/rh-alert.js 184 B
./react/rh-audio-player/rh-audio-player-about.js 191 B
./react/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 214 B
./react/rh-audio-player/rh-audio-player-subscribe.js 196 B
./react/rh-audio-player/rh-audio-player.js 183 B
./react/rh-audio-player/rh-cue.js 195 B
./react/rh-audio-player/rh-transcript.js 207 B
./react/rh-avatar/rh-avatar.js 173 B
./react/rh-back-to-top/rh-back-to-top.js 183 B
./react/rh-badge/rh-badge.js 174 B
./react/rh-blockquote/rh-blockquote.js 179 B
./react/rh-button/rh-button.js 174 B
./react/rh-card/rh-card.js 172 B
./react/rh-code-block/rh-code-block.js 181 B
./react/rh-cta/rh-cta.js 170 B
./react/rh-dialog/rh-dialog.js 203 B
./react/rh-footer/rh-footer-block.js 184 B
./react/rh-footer/rh-footer-copyright.js 187 B
./react/rh-footer/rh-footer-links.js 185 B
./react/rh-footer/rh-footer-social-link.js 193 B
./react/rh-footer/rh-footer-universal.js 188 B
./react/rh-footer/rh-footer.js 174 B
./react/rh-footer/rh-global-footer.js 185 B
./react/rh-menu/rh-menu.js 173 B
./react/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 217 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 205 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu.js 199 B
./react/rh-navigation-secondary/rh-navigation-secondary-overlay.js 201 B
./react/rh-navigation-secondary/rh-navigation-secondary.js 213 B
./react/rh-pagination/rh-pagination.js 178 B
./react/rh-site-status/rh-site-status.js 181 B
./react/rh-skip-link/rh-skip-link.js 181 B
./react/rh-spinner/rh-spinner.js 175 B
./react/rh-stat/rh-stat.js 171 B
./react/rh-subnav/rh-subnav.js 175 B
./react/rh-surface/rh-surface.js 175 B
./react/rh-table/rh-sort-button.js 213 B
./react/rh-table/rh-table.js 174 B
./react/rh-tabs/rh-tab-panel.js 181 B
./react/rh-tabs/rh-tab.js 187 B
./react/rh-tabs/rh-tabs.js 174 B
./react/rh-tag/rh-tag.js 182 B
./react/rh-tile/rh-tile-group.js 183 B
./react/rh-tile/rh-tile.js 194 B
./react/rh-timestamp/rh-timestamp.js 176 B
./react/rh-tooltip/rh-tooltip.js 175 B
./rhds.min.js 90.5 kB

compressed-size-action

@markcaron markcaron added the needs revisiting Outdated issues worth revisiting in the future label Feb 7, 2024
@markcaron
Copy link
Collaborator

@bennypowers looks like this PR is waiting for copy/content, eh?

TBD on who picks this up.

CC: @hellogreg @coreyvickery @marionnegp

@markcaron markcaron added docs Improvements or additions to documentation needs discussion This issue needs further discussion labels Feb 8, 2024
@markcaron markcaron self-assigned this Feb 8, 2024
@bennypowers bennypowers marked this pull request as ready for review May 23, 2024 05:43
@bennypowers
Copy link
Member Author

@marionnegp compare https://deploy-preview-1188--red-hat-design-system.netlify.app/patterns/card/ with https://deploy-preview-1188--red-hat-design-system.netlify.app/elements/card/guidelines/

It seems to me we should move some content around and elaborate on the patterns listen in guidelines. WDYT?

@marionnegp
Copy link
Collaborator

Yeah, I think we can move the "Variants" section to the card pattern page. We might also want to add a link to the card pattern page in the "Other components" section of the card element docs.

@bennypowers
Copy link
Member Author

@marionnegp I've elaborated as much as seemed reasonable, but it wasn't clear to me how to implement most of the patterns listed here. i think we should pair on this, or if you have examples of the patterns in question, please link to them

Copy link
Collaborator

@marionnegp marionnegp left a comment

Choose a reason for hiding this comment

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

Just some early feedback...

  • I think the "Usage" section should be moved to a separate "Guidelines" tab, even if there's not a lot of info. Just helps to make the subnavs more consistent.
  • Are code blocks getting added for each pattern?
  • What do you think about having the code block span the entire accordion/disclosure and having it show more by default?
    Screenshot 2024-07-22 at 12 41 18 PM
  • The image card demo with the code blocks looks nice!

@markcaron markcaron removed needs revisiting Outdated issues worth revisiting in the future needs discussion This issue needs further discussion labels Jul 25, 2024
@markcaron
Copy link
Collaborator

I believe I've added all the Card patterns except for the Video Card (which I commented out in the code).

@adamjohnson @zeroedin, We will need to discuss how to handle the play button and thumbnail in the pattern along with the described Modal solution. Do we wait for <rh-video> to handle the play button/thumbnail pattern?

@marionnegp can you look through the patterns and let me know which ones are missing?
Do we need to tackle the light gray versions of the cards too?

@marionnegp
Copy link
Collaborator

@markcaron, I don't think you'd need to include the light gray versions. <rh-card> should be able to use any surface color, so that would be a lot of demos to include. 😅

Looks like you've captured all the patterns, except for the one with the video thumbnail, which was discussed during today's office hours. I don't see that card pattern very often, so it might be ok to include in a minor release or the next milestone.

The bar for the title card is too tall. Is this an issue with <rh-card>?
Screenshot 2024-07-29 at 9 46 37 AM

@zeroedin
Copy link
Collaborator

zeroedin commented Jul 29, 2024

The bar for the title card is too tall. Is this an issue with <rh-card>?

All headers as default have a margin-block-end on them (flow layer of uxdot styles).

Because the h4 is slotted in the component and is not directly styled to override page styles this style bleeds through to the element.

The way to fix it would be either:

  1. in the pattern to explicitly pass a margin: 0; on rh-card#example-title-bar-card h4[slot]
  2. in the component shadow styles slotted header style update margin: unset with an !important to override page style margins always in #header ::slotted(:is(h1, h2, h3, h4, h5, h6))

@markcaron
Copy link
Collaborator

@marionnegp, I would think the latter of @zeroedin's proposed solutions is the answer to fixing the title bar height.

@markcaron
Copy link
Collaborator

markcaron commented Jul 29, 2024

@zeroedin @marionnegp looks like we'll just need to make a change here to fix this in Charmander. Will do in another PR

@markcaron markcaron removed the request for review from coreyvickery August 1, 2024 14:39
@marionnegp marionnegp self-requested a review August 6, 2024 14:21
Copy link
Collaborator

@marionnegp marionnegp left a comment

Choose a reason for hiding this comment

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

I'm seeing a missing space in the image card example.
Screenshot 2024-08-06 at 11 21 29 AM

Is it possible to do add a bottom margin for something like #header ::slotted(img) + #footer? Maybe this is something to tackle in another PR?

@markcaron
Copy link
Collaborator

I'm seeing a missing space in the image card example.
Is it possible to do add a bottom margin for something like #header ::slotted(img) + #footer? Maybe this is something to tackle in another PR?

Yeah. I left a comment in the code to solve later, once we have the image slot and other adjustments being made to card in Charmander, this should resolve itself.

image

The following PRs (in Charmander) will resolve the issues mentioned above:

@bennypowers
Copy link
Member Author

may i suggest this path forward?

  1. the conflicts in staging/charmander should be resolved
  2. this branch should be rebased to staging/charmander
  3. there resulting conflicts should be resolved

@markcaron markcaron merged commit bdef81f into main Aug 6, 2024
8 checks passed
@markcaron markcaron deleted the docs/card-pattern-explanation branch August 6, 2024 19:00
@markcaron
Copy link
Collaborator

markcaron commented Aug 6, 2024

@bennypowers sorry, for some reason I didn't see your last comment before merging.

We've got a couple docs and patterns updates that were dependent on these changes, so hopefully this doesn't mess up staging/charmander

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation
Projects
Status: Done ☑️
Development

Successfully merging this pull request may close these issues.

[bug] <rh-card> docs: guidelines page
4 participants