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

feat: <RelatedContent /> component #731

Merged
merged 23 commits into from
Oct 7, 2022

Conversation

nandereck
Copy link
Contributor

@nandereck nandereck commented Sep 28, 2022

🎟️ Asana Task
πŸ” Preview Link


Description

Adds new <RelatedContent /> component. One thing to note is that this does not include the initially proposed dropdown. Instead, we've replaced with a CTA (using the <StandaloneLink />).

Figma

PR Checklist πŸš€

Items in this checklist may not may not apply to your PR, but please consider each item carefully.

  • Add Asana and Preview links above.
  • Conduct thorough self-review.
  • Add or update tests as appropriate.
  • Conduct reasonable cross browser testing for both compatibility and responsive behavior (We have a Sauce Labs account for this, if you don't have access, just ask!).
  • Conduct reasonable accessibility review (use the WAS as a guide or an axe browser plugin until we establish more formal checks).
  • Identify (in the description above) and document (add Asana tasks on this board) any technical debt that you're aware of, but are not addressing as part of this PR.

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2022

πŸ¦‹ Changeset detected

Latest commit: 63aac7a

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

This PR includes changesets to release 1 package
Name Type
@hashicorp/react-related-content 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

@vercel
Copy link

vercel bot commented Sep 28, 2022

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
react-components βœ… Ready (Inspect) Visit Preview Oct 5, 2022 at 6:35PM (UTC)

@nandereck nandereck changed the title Create RelatedContent WPL component feat: <RelatedContent /> component Sep 29, 2022
@nandereck nandereck changed the title feat: <RelatedContent /> component feat: <RelatedContent /> component Sep 29, 2022
@nandereck nandereck changed the title feat: <RelatedContent /> component feat: <RelatedContent /> component Sep 29, 2022
@nandereck nandereck marked this pull request as ready for review September 29, 2022 22:43
packages/related-content/types.ts Outdated Show resolved Hide resolved
packages/related-content/style.module.css Outdated Show resolved Hide resolved
packages/related-content/props.js Outdated Show resolved Hide resolved
packages/related-content/props.js Outdated Show resolved Hide resolved
packages/related-content/types.ts Outdated Show resolved Hide resolved
--columns: 2;
}
@media (--large) {
--columns: 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm expectations, if there are three cards provided, we still render 4 columns across vs 3 columns across?

Copy link
Contributor

Choose a reason for hiding this comment

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

CleanShot 2022-10-03 at 10 42 25@2x

Bottom example rendering 3 columns vs 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm expectations, if there are three cards provided, we still render 4 columns across vs 3 columns across?

My understanding is that this component can only render 4 cards.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, so to make use of RelatedContent, folks will need to provide at least 4 cards. Or it's ok that less than 4 cards are provided, they just will still render within 4 columns? I could see the need to use less based on some of content the content I have seen on .io sites.

If we've decided that is a requirement, what do you think about adding a length check to the cards array that at least 4 cards are provided then or update types to reflect 4 cards?

cards: [RelatedContentCardProps, RelatedContentCardProps, RelatedContentCardProps, RelatedContentCardProps]

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@nandereck nandereck Oct 4, 2022

Choose a reason for hiding this comment

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

Interesting, so to make use of RelatedContent, folks will need to provide at least 4 cards. Or it's ok that less than 4 cards are provided, they just will still render within 4 columns? I could see the need to use less based on some of content the content I have seen on .io sites.

FWIW - I think this component should allow for multiple cards and not be limited to only 4. I wonder if it makes sense to enable that and make adjustments later. Especially if we're already seeing deviations from the initial requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nandereck I agree: I think it makes sense to go with how it's currently implemented (render with 4 columns regardless of count, do not limit to 4 cards minimum). Especially with deviations already existing.

As an aside, this implementation is aligned with how local related resources components are currently implemented, an example being on our thank you page πŸ‘

Copy link
Contributor

@alexcarpenter alexcarpenter left a comment

Choose a reason for hiding this comment

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

πŸ‘πŸΌ

Copy link
Contributor

@EnMod EnMod left a comment

Choose a reason for hiding this comment

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

πŸ’―

@nandereck nandereck merged commit 2a26b0b into main Oct 7, 2022
@nandereck nandereck deleted the nels/related-content/create-component branch October 7, 2022 17:08
@hashibot-web hashibot-web mentioned this pull request Oct 7, 2022
nandereck pushed a commit that referenced this pull request Oct 7, 2022
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.

# Releases
## @hashicorp/react-related-content@0.2.0

### Minor Changes

-   [#731](#731) [`2a26b0bf`](2a26b0b) Thanks [@nandereck](https://github.com/nandereck)! - Create RelatedContent component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants