Skip to content

Conversation

dromo77
Copy link
Contributor

@dromo77 dromo77 commented Nov 11, 2021

Overview

This PR adds a Media Group object. It should be helpful for aligning images/content on pages like the Case Study Listing page.

Screenshots

Screen Shot 2021-11-11 at 11 12 16 AM

Testing


@dromo77 dromo77 requested a review from a team November 11, 2021 19:18
@changeset-bot
Copy link

changeset-bot bot commented Nov 11, 2021

🦋 Changeset detected

Latest commit: 7fed1a7

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

This PR includes changesets to release 1 package
Name Type
@cloudfour/patterns 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 Nov 11, 2021

✔️ Deploy Preview for cloudfour-patterns ready!

🔨 Explore the source changes: 7fed1a7

🔍 Inspect the deploy log: https://app.netlify.com/sites/cloudfour-patterns/deploys/6192b555713b880008fcd832

😎 Browse the preview: https://deploy-preview-1579--cloudfour-patterns.netlify.app

@tylersticka
Copy link
Member

@dromo77 Thanks for tackling this challenging layout problem.

I'm not sure I understand what o-media-group__item does that o-media does not, or what o-media-group does that o-deck does not. I assume I'm missing something between the way the two interact?

@dromo77
Copy link
Contributor Author

dromo77 commented Nov 11, 2021

I'm not sure I understand what o-media-group__item does that o-media does not, or what o-media-group does that o-deck does not. I assume I'm missing something between the way the two interact?

@tylersticka I ran into a couple issues trying to use o-media and o-deck as a solution. In the prototype the content splits into two columns at the l breakpoint. o-deck automatically creates columns, so there were multiple columns before the l breakpoint. The content looked a little squished.

o-media doesn't size the object and also uses flex. I thought about some sort of modifier that uses CSS Grid. This would help size the object and could also be used to align objects vertically, similar to what the o-media-group__item is doing. However, the alignment and grid size of the object are different on small screens versus large screens. I wasn't sure how to handle the different sizes/alignment needed.

I'm open to other solutions, but wanted to give some context to the problems I ran into using existing patterns.

@tylersticka
Copy link
Member

o-deck automatically creates columns, so there were multiple columns before the l breakpoint. The content looked a little squished.

I assume that was true even with the o-deck--2-column@lmodifier. Perhaps we should add 1-column modifiers to o-deck? Would that solve the issue?

o-media doesn't size the object and also uses flex. I thought about some sort of modifier that uses CSS Grid. This would help size the object and could also be used to align objects vertically, similar to what the o-media-group__item is doing. However, the alignment and grid size of the object are different on small screens versus large screens. I wasn't sure how to handle the different sizes/alignment needed.

Reading the comment in the media.scss file, it seems like I used Flexbox to write it just because it was simple. But that's not a very compelling reason to keep using Flexbox. If there's a good reason to use CSS Grid instead, we should totally do that.

From there, there are probably a few ways of handling those case study image widths. Perhaps we could make one or more o-media modifiers that enforce a certain object column width?

Or, honestly, I don't mind the different media columns being different as much as I used to. If we find that's taking an outsized amount of effort to componentize, maybe it isn't completely necessary after all? What do you think?

@dromo77
Copy link
Contributor Author

dromo77 commented Nov 11, 2021

If we find that's taking an outsized amount of effort to componentize, maybe it isn't completely necessary after all?

I'll try using our existing patterns with the changes you mentioned. I don't think it will take too much time to add o-media modifiers, but if I find otherwise we can consider different sized columns.

@dromo77
Copy link
Contributor Author

dromo77 commented Nov 12, 2021

@tylersticka I made several changes:

  1. I updated the Case Study Listing prototype to use patterns so we can see what it looks like in action.
  2. I updated the o-deck column modifier so you can specify when you want 1 column.
  3. I updated o-media to use CSS grid when supported. I think I did it correctly. I haven't noticed any regressions, but would appreciate another pair of eyes on it.
  4. I added a modifier to specify the size of o-media__object relative to o-media__content. I want to make sure the modifier name and documentation makes sense.
  5. I added another modifier to o-media to fix some alignment issues I noticed in the prototype.

@dromo77 dromo77 requested a review from tylersticka November 12, 2021 19:14
Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

It looks like some of the prototype images have been copied from src/prototypes/case-study-listings/images to static/media. That's fine, but can we remove the duplicates so they are only in one place?

@dromo77 dromo77 requested a review from tylersticka November 12, 2021 22:45
Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

Apparently I wrote a comment on Friday that I never actually posted. Sorry about that. 🤦

Comment on lines 89 to 99
The `o-deck--align-start` modifier can be used to top-align deck items.

<Canvas>
<Story
name="Alignment"
height="400px"
args={{ class: 'o-deck--align-start' }}
>
{articlesStory.bind({})}
</Story>
</Canvas>
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a demo that more clearly shows the use case, as well as more context for why one would use it versus the default behavior? It isn't clear to me how the demo differs visually from the others as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tylersticka I updated the example, let me know if it makes more sense 🙂

@dromo77 dromo77 requested a review from tylersticka November 15, 2021 19:32
@dromo77 dromo77 merged commit 98ffb8b into v-next Nov 15, 2021
@dromo77 dromo77 deleted the feature/columns branch November 15, 2021 21:02
@github-actions github-actions bot mentioned this pull request Nov 15, 2021
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.

Column content alignment pattern
2 participants