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

fix(card): adding thumbnail slot, header styling for image / header combo #1207

Merged
merged 6 commits into from
Sep 20, 2023
Merged

fix(card): adding thumbnail slot, header styling for image / header combo #1207

merged 6 commits into from
Sep 20, 2023

Conversation

brianferry
Copy link
Collaborator

@brianferry brianferry commented Aug 31, 2023 •
edited
Loading

What I did

  1. Added a thumbnail slot to the rh-card component so that users can have both the thumbnail image and an heading tag insider of the <header> block in the shadowdom.
  2. Styled the header block so that the image and the header (if they're both present) stack vertically.

@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 902525c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rhds/elements Minor

Not sure what this means? Click here to learn what changesets are.

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

@netlify
Copy link

netlify bot commented Aug 31, 2023 •
edited
Loading

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

Name Link
🔨 Latest commit 902525c
🔍 Latest deploy log https://app.netlify.com/sites/red-hat-design-system/deploys/6509e0d223e7670007f1178c
😎 Deploy Preview https://deploy-preview-1207--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.

@brianferry brianferry changed the title fix(card): adding thumbnail slot, header styles feat(card): adding thumbnail slot, header styling for image / header combo Aug 31, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 31, 2023 •
edited
Loading

Size Change: -29 B (0%)

Total Size: 189 kB

Filename Size Change
./elements/rh-card/rh-card.js 1.88 kB -16 B (-1%)
./rhds.min.js 77.4 kB -13 B (0%)
ℹ️ View Unchanged
Filename Size
./elements/rh-accordion/rh-accordion-header.js 1.98 kB
./elements/rh-accordion/rh-accordion-panel.js 1.24 kB
./elements/rh-accordion/rh-accordion.js 1.17 kB
./elements/rh-alert/rh-alert.js 4.24 kB
./elements/rh-audio-player/rh-audio-player-about.js 1.85 kB
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.51 kB
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.43 kB
./elements/rh-audio-player/rh-audio-player.js 14.2 kB
./elements/rh-audio-player/rh-cue.js 2 kB
./elements/rh-audio-player/rh-transcript.js 2.94 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.88 kB
./elements/rh-badge/rh-badge.js 862 B
./elements/rh-blockquote/rh-blockquote.js 1.96 kB
./elements/rh-button/rh-button.js 3.77 kB
./elements/rh-code-block/rh-code-block.js 1.18 kB
./elements/rh-context-provider/rh-context-provider.js 185 B
./elements/rh-cta/rh-cta.js 4.04 kB
./elements/rh-dialog/rh-dialog.js 1.6 kB
./elements/rh-dialog/yt-api.js 614 B
./elements/rh-footer/rh-footer-block.js 765 B
./elements/rh-footer/rh-footer-copyright.js 362 B
./elements/rh-footer/rh-footer-links.js 1.18 kB
./elements/rh-footer/rh-footer-social-link.js 960 B
./elements/rh-footer/rh-footer-universal.js 4.07 kB
./elements/rh-footer/rh-footer.js 5.07 kB
./elements/rh-footer/rh-global-footer.js 250 B
./elements/rh-menu/rh-menu.js 1.02 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.31 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 4.76 kB
./elements/rh-navigation-secondary/test/fixtures.js 851 B
./elements/rh-pagination/rh-pagination.js 4.45 kB
./elements/rh-spinner/rh-spinner.js 1.51 kB
./elements/rh-stat/rh-stat.js 2.24 kB
./elements/rh-subnav/rh-subnav.js 2.84 kB
./elements/rh-tabs/rh-tab-panel.js 789 B
./elements/rh-tabs/rh-tab.js 1.81 kB
./elements/rh-tabs/rh-tabs.js 1.86 kB
./elements/rh-tag/rh-tag.js 1.73 kB
./elements/rh-timestamp/rh-timestamp.js 974 B
./elements/rh-tooltip/rh-tooltip.js 1.07 kB
./lib/context/color/consumer.js 1.15 kB
./lib/context/color/controller.js 1.11 kB
./lib/context/color/provider.js 1.99 kB
./lib/context/event.js 598 B
./lib/context/headings/consumer.js 711 B
./lib/context/headings/controller.js 1.14 kB
./lib/context/headings/provider.js 1.23 kB
./lib/DirController.js 569 B
./lib/elements/rh-context-picker/rh-context-picker.js 1.16 kB
./lib/elements/rh-context-provider/rh-context-provider.js 746 B
./lib/functions.js 175 B
./lib/I18nController.js 1.38 kB
./lib/ScreenSizeController.js 856 B

compressed-size-action

@brianferry
Copy link
Collaborator Author

@coreyvickery @marionnegp

Need some feedback here with regards to the header sizing. Not currently limiting the header image size here to give room for the image and the header tag. Not sure if there should be a hard limit on that image size.

@nikkimk @hellogreg

Both the thumbnail image and the heading tag are in the

block which seems okay but wasn't sure if the placement of these would cause any problems? (Image tag above heading tag)

@hellogreg
Copy link
Collaborator

Both the thumbnail image and the heading tag are in the block which seems okay but wasn't sure if the placement of these would cause any problems? (Image tag above heading tag)

If the image is non-decorative (i.e., needs alt text), I think you'd ideally want it to be after its associated heading in the source, even if it's first visually. That way users of assistive tech navigating by headings will experience it as part of that section. (In other words, if the image comes before the heading, a screen reader user may never know it's there.)

This is a case where the logical reading order doesn't necessarily match the visual reading order.

W3C WAI tip example

Where it gets tricky is if both the image and the heading are links (as I know has been done when implementing cards in the past, and maybe present). Then visual focus order could be moving in an unexpected direction: tab focus moving up from the heading to the image. But I think we really shouldn't be having two or three links (image, heading, and CTA) to the same destination in cards anyway.

@marionnegp
Copy link
Collaborator

@coreyvickery @brianferry, I'm not sure if we need a hard limit on the image size. Might be better to add guidelines to documentation if we need to make a recommendation.

@bennypowers bennypowers linked an issue Sep 7, 2023 that may be closed by this pull request
[bug] headings on full width image card aren't working #1205
Closed
@bennypowers
Copy link
Member

is it possible to do this without adding or removing a new slot name?

@brianferry
Copy link
Collaborator Author

is it possible to do this without adding or removing a new slot name?

We can instruct the user to include both their image and heading element in the header slot and update the css to show all the items in the header. Then update our .full pattern css to include the styles needed.

Pretty minor changes with that method

@brianferry brianferry marked this pull request as ready for review September 14, 2023 20:23
bennypowers
bennypowers approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

Nice, minimal change that gets the job done :)

@bennypowers
Copy link
Member

@diwanshi does this address your issue in #1205 ?

@diwanshi
Copy link
Collaborator

@bennypowers @brianferry yes that works for us.

@bennypowers bennypowers changed the title feat(card): adding thumbnail slot, header styling for image / header combo fix(card): adding thumbnail slot, header styling for image / header combo Sep 18, 2023
@bennypowers
Copy link
Member

@brianferry can you add a patch changeset? Then we're good to merge

@brianferry
Copy link
Collaborator Author

@bennypowers added

@brianferry brianferry merged commit 39e76fc into main Sep 20, 2023
@brianferry brianferry deleted the fix/rh-card-thumbnail-and-header branch September 20, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennypowers bennypowers bennypowers approved these changes

@marionnegp marionnegp Awaiting requested review from marionnegp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] headings on full width image card aren't working
5 participants
@brianferry @hellogreg @marionnegp @bennypowers @diwanshi