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

Refactoring to support downstream work in UIDS Base card updates #847

Merged
merged 55 commits into from
Apr 18, 2023

Conversation

pyrello
Copy link
Contributor

@pyrello pyrello commented Jan 9, 2023

Example here:

@pyrello pyrello changed the title Updates to support work in uiowa/uiowa#5849 Refactoring to support downstream work in UIDS Base card updates Jan 9, 2023
@pyrello
Copy link
Contributor Author

pyrello commented Jan 11, 2023

When testing these updates in SiteNow, we noticed there seems to be an issue with precedence where the media shape classes get precedence over the media size classes and affect whether the sizes actually get rendered correctly.

Behavior can be seen here: http://localhost:6006/?path=/story/components-card--default&args=orientation:left;media_size:small

@pyrello
Copy link
Contributor Author

pyrello commented Jan 20, 2023

We just discussed the issue in #847 (comment). It seems that we are running into issues with our media--small, etc. classes having a flex property built into them. After talking it through the media size property doesn't really seem to make a lot of sense. It should really be a property of the component that is implementing a media element. Therefore, we are going to refactor to move this from media into the card. When we eventually want to do the same thing with block quotes, we can abstract it out.

@bspeare
Copy link
Contributor

bspeare commented Jan 20, 2023

If we go with the changes in b5849be, In SiteNow we would not want to render a media--large class on stacked widescreen cards. If that's not desirable, then we could add a line in for media--large.media--widescreen {width: 100%;} to allow the media--large class to work with stacked cards.

@bspeare
Copy link
Contributor

bspeare commented Jan 23, 2023

Since our "small" image size is our default in SiteNow and used in layout columns as lists do we want to set a fixed pixel value for the small size and leave medium and large as percentages? As you can see in these screenshots, small can get very small.

Screen Shot 2023-01-23 at 3 45 37 PM

Screen Shot 2023-01-23 at 3 44 43 PM

Screen Shot 2023-01-23 at 3 44 30 PM

@pyrello
Copy link
Contributor Author

pyrello commented Jan 24, 2023

Since our "small" image size is our default in SiteNow and used in layout columns as lists do we want to set a fixed pixel value for the small size and leave medium and large as percentages? As you can see in these screenshots, small can get very small.

Maybe we set a min-width for the small size? I'd like to take a look at this using the new grid tool to understand it a bit better. I think we probably need some rules similar to what we developed for inline sizing, but specifically for card images.

@bspeare
Copy link
Contributor

bspeare commented Jan 24, 2023

@pyrello that's a good idea!

@bspeare

This comment was marked as outdated.

@bspeare

This comment was marked as outdated.

@bspeare bspeare marked this pull request as ready for review April 18, 2023 19:42
@pyrello pyrello merged commit d6ca8f8 into 4.x Apr 18, 2023
@pyrello pyrello deleted the more_card_updates branch April 18, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants