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

Lets have a card #698

Merged
merged 29 commits into from
Jun 22, 2022
Merged

Lets have a card #698

merged 29 commits into from
Jun 22, 2022

Conversation

pyrello
Copy link
Contributor

@pyrello pyrello commented May 27, 2022 •
edited
Loading

Resolves #696

To test

  • Visit https://uids.brand.uiowa.edu/branches/lets_have_a_card/
  • Review card functionality against items in 4.x - Create a card component #696.
  • Click through each card example and then toggle the different settings for each one to see if you can find any options that don't look right.
  • I removed the container around the card because it was getting in the way of testing the media alignment options. I recommend using the viewport button to set it to a smaller width for testing the default card without a media alignment option selected.

pyrello and others added 10 commits May 27, 2022 16:02
@pyrello
Added card component. Started with slots for almost everything to see…
bd75f3d 
… how that felt in my head.
@pyrello
Move image from a slot to property. Chances are the only thing that i…
70abb14 
…s going to change with a card image is the URL, if it exists, so we don't need the freeform slot to pass in HTML. This decision may end up being reversed in the future.
@pyrello
Cleaned up a little sloppy HTML. Rethinking the slots.
deafbbe
@pyrello
Adding options to card story, started transitioning over the CSS.
3219578
@pyrello
Added option to have enclosed card.
8152237
@GaryRidgway
starting on some card customization
51c43b8
@GaryRidgway
fumbling around with some things, we should definitely refactor if ne…
8b0a7d6 
…eded
@pyrello
Adjusting the approach used for outline padding on the card.
f9e8df3
@GaryRidgway
getting buttons to work!
da476ac
@pyrello
More refactoring to use 'url' as prop name. Removed 'action' slot fro…
868ab55 
…m card. Added 'link_text' prop to card. Added computed values to determine button functionality.
@pyrello pyrello mentioned this pull request Jun 14, 2022
Closed
5 tasks
pyrello and others added 16 commits June 14, 2022 09:48
@pyrello
Refactored card stories to provide clearer examples.
3e058bb
@pyrello
Refactoring to add media padded property to cards.
886f6d3
@pyrello
Made the card with outline the default and added a borderless option.
79ad56c
@pyrello
Refactored card styles to move styles we are keeping to places that m…
f8bf92e 
…ake more sense and removing styles that no longer are necessary.
@pyrello
Refactored background property into a shared resource.
5c219b3
@pyrello
Added background styles. Added SB argTypes to background mixin. Other…
bcdc63a 
… various fixes.
@pyrello
Add card export. Add more to borderless card example.
063c222
@pyrello
New build.
62960ab
@bspeare
Added pseudo button
8121972
@bspeare
Added left or right alignement options
457c506
@bspeare
Trying out image cover property for edge to edge images
3be2173
@GaryRidgway
small changes
058bd5f
@pyrello
Add background color options for canvas to allow more flexibility in …
5a1e4f3 
…testing.
@pyrello
Removed styles that don't seem to be necessary anymore. Fixed a coupl…
0bc8542 
…e things that got broken. Commented out a bunch of stuff that might need more scrutiny.
@pyrello
I believe that these rules are no longer needed because of the change…
9a0cd1d 
…s to the way we are handling padding.
@pyrello
Removed more rules related to adjusting margin/padding based on situa…
a5f91b7 
…tion outside the card. Should not be needed anymore based on changed to the way padding is handled.
@pyrello pyrello linked an issue Jun 17, 2022 that may be closed by this pull request
4.x - Create a card component #696
Closed
14 tasks
@pyrello pyrello marked this pull request as ready for review June 17, 2022 20:37
@pyrello pyrello added the 4.x 4.x version label Jun 17, 2022
@pyrello pyrello requested a review from a team June 17, 2022 21:35
@joewhitsitt
Copy link
Contributor

👀

@joewhitsitt joewhitsitt self-assigned this Jun 21, 2022
@joewhitsitt
Copy link
Contributor

reviewing

joewhitsitt
joewhitsitt approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@joewhitsitt joewhitsitt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In terms of the card (minus media), I tried several combinations and each seem to work as I expected however, a couple stood out to me and I am not sure if they are important or not:

When I click basic-card--linked-card-without-title (removed link text) - I assume that is for SiteNow or other implementation to validate and not storybook.

When I clicked basic-card--borderless-card (mobile sized), I didn't notice a change in media padding. This was discussed in the huddle I was in earlier, but I am foggy about what was decided here.

Generally curious about how components will stack in Storybook and if we will be able to modify media or button settings while viewing the card component.

There is a lot of other stuff in this PR that I assume is safe to ignore for the time being since it wasn't the purview of this PR.

The fact that this can be reviewed in browser at this time and that certain card properties are handled, I think this is a good step forward!

@joewhitsitt joewhitsitt removed their assignment Jun 21, 2022
@pyrello pyrello merged commit 796854f into 4.x Jun 22, 2022
@pyrello pyrello deleted the lets_have_a_card branch June 22, 2022 13:32
This was referenced Oct 11, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joewhitsitt joewhitsitt joewhitsitt approved these changes

Assignees
No one assigned
Labels
4.x 4.x version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4.x - Create a card component
4 participants
@pyrello @joewhitsitt @GaryRidgway @bspeare