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

Finish card component #785

Merged
merged 94 commits into from
Oct 27, 2022
Merged

Finish card component #785

merged 94 commits into from
Oct 27, 2022

Conversation

joewhitsitt
Copy link
Contributor

@joewhitsitt joewhitsitt commented Oct 12, 2022

Resolves #781

To test

Remaining tasks

  • logic for links/buttons
  • Account for click-ally.js
  • Allow headline to change between Roboto (default) and Zilla (serif)
    • Need to set specific font-size: 1.5rem; for card .headline
  • Cleanup card story argTypes labels and categories
  • Add the circle button
  • Add card button position
  • Responsive video in card for square and widescreen sizes
  • Set max-width pixel value on small, medium, and large media sizes
    • We added container queries for setting these values
  • Final SCSS cleanup 4.x - Finish card component #781 (comment)

Potential additional tasks

  • Hide Image Using Font Awesome Icon story
  • Clear up the "border double negative" hold up for now
  • Don't assume 1:1 aspect ratio for circle images and provide built-in object fit
  • Move align to bottom to display options group
  • ~~No corresponding "collection" items found. (in comparing to current uids) ~~ not in scope
  • Issue with Align button to bottom and Centered combinations

Notes for SiteNow

  • Media-align left or right with a circle selected needs media—padded modifier unless ‘borderless’ is selected.
  • "Padded" option should only be available within the "stacked" format, padding is added by default for bordered and background color card in left/right format.
  • When circle image is set within "stacked", media—padded needs to be applied.
  • "Padded" option does not work if "borderless" is applied.

@joewhitsitt joewhitsitt changed the base branch from 3.x to 4.x October 12, 2022 20:46
@e-marie-w e-marie-w self-assigned this Oct 26, 2022
@makurj
Copy link
Contributor

makurj commented Oct 26, 2022

Tried circle image with left and right orientation. Didn't seem to work:

image

image

Noticed similar behavior with other images crop with left/right orientation. It didn't seem to switch.

@bspeare
Copy link
Contributor

bspeare commented Oct 26, 2022

Stacked, circle, padded or not: image size doesn't change between medium and large.

When circle image is set within "stacked", the media—padded class needs to be applied. I assume we will be adding the media--padded class programmatically in SiteNow when that selection is made?

@bspeare
Copy link
Contributor

bspeare commented Oct 26, 2022

Tried circle image with left and right orientation. Didn't seem to work:

@makurj Try changing the viewport to "Tablet", all mobile styles are forced into the stacked format currently.

Screen Shot 2022-10-26 at 9 15 11 AM

@makurj
Copy link
Contributor

makurj commented Oct 26, 2022

That worked! Thanks, Ben.

@makurj
Copy link
Contributor

makurj commented Oct 26, 2022

"Image Using Font Awesome Icon": The icon disappears on "large mobile" with any shape other than "widescreen", no matter the orientation. It appears fine for tablet viewport.

@bspeare
Copy link
Contributor

bspeare commented Oct 26, 2022

"Image Using Font Awesome Icon": The icon disappears on "large mobile" with any shape other than "widescreen", no matter the orientation. It appears fine for tablet viewport.

I'd be in favor of hiding this story for now. I don't think this story is finished and it's not something we are using in SiteNow currently.

@e-marie-w
Copy link
Contributor

Could we switch the "Display Options > No border" to "Display Options > Border > True/False"? The double negative could cause confusion.

So this did confuse me! Especially because a different section has Media > Border > True/False. I'm thinking we set up our default card so that all extra options are False. And since we wanted a card border but no media border for default, the card border option ended up being No border set as False and media border just Border set as False. So, it makes sense why we did it this way, but I still think it's confusing to users (both the double negative and the options not matching) and worth discussing changing.

@e-marie-w
Copy link
Contributor

e-marie-w commented Oct 26, 2022

Not sure this is in scope, as it wasn't a combination option on the old UIDS site, but I wanted to document it for the future (assuming it isn't expected behavior).

When I turn on Align button to bottom and Centered when in tablet mode or have reset the viewport, the centering no longer takes effect. It varies between the two - reset viewport (computer view I assume?) aligns everything to the left. In tablet view, the content remains centered, but the title and link are left-aligned. This happens in all orientations - left, right, and stacked.

Also happening with the above combinations is that the subtitle and meta text behave strangely when both added. Subtitle by itself is left-aligned like the title and link. But when I start typing meta text the subtitle starts moving to the right. Meta text is left-aligned with and without the subtitle.

Edit: I figured out screenrecording! So you can see it instead of having to recreate it yourself.

Screen.Recording.2022-10-26.at.11.38.58.AM.mov

@e-marie-w
Copy link
Contributor

Not in scope and not an error or loss of functionality but a change that may cause confusion: if the card has link text the Props category appears before Display Options. When you remove link text it appears after Display Options. aka I thought I broke something until I scrolled down.

@joewhitsitt joewhitsitt self-assigned this Oct 26, 2022
@joewhitsitt
Copy link
Contributor Author

pulling down

@joewhitsitt
Copy link
Contributor Author

@media (min-width: 768px) .card--button-align-bottom .card__body seems to override text-align: center; with align-items: flex-start;

should this be a :not somehow?

@bspeare
Copy link
Contributor

bspeare commented Oct 26, 2022

@joewhitsitt I think we need to set a default margin at

on the bttn to something like:

footer {
    .bttn {
      margin-top: 2rem;
    }
  }

Then this would work. I liked targeting the <footer> for the margin on this but I think we need a default height set on the bttn.

@joewhitsitt
Copy link
Contributor Author

@bspeare i made that margin change, but there is still this alignment difference between 3.x and 4.x:

Screen Shot 2022-10-26 at 4 08 14 PM

https://uids.brand.uiowa.edu/components/detail/card--centered-enclosed.html

@bspeare
Copy link
Contributor

bspeare commented Oct 26, 2022

@joewhitsitt can you share a link that contains the args in that screenshot?

@bspeare
Copy link
Contributor

bspeare commented Oct 26, 2022

I think I found it: http://localhost:6006/?path=/story/components-card--person-profile&args=button_align_bottom:true;borderless:false;centered:true;orientation:

@bspeare
Copy link
Contributor

bspeare commented Oct 27, 2022

@joewhitsitt I think #785 (comment) is fixed by moving the <footer> outside of card--body.

@joewhitsitt
Copy link
Contributor Author

This might have introduced a new oddity:

http://localhost:6006/?path=/story/components-card--default&args=button_align_bottom:true;orientation:left;media_border:true;media_shape:circle;media_padded:true

Screen Shot 2022-10-26 at 4 08 14 PM

@joewhitsitt
Copy link
Contributor Author

As a team we reviewed the feedback that was part of this PR and have fixed the items in scope.

@joewhitsitt joewhitsitt merged commit 2a20b04 into 4.x Oct 27, 2022
@joewhitsitt joewhitsitt deleted the finish-card-component branch October 27, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x 4.x version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.x - Finish card component
6 participants