-
Notifications
You must be signed in to change notification settings - Fork 58
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(card_image): Improve card_image()
API and usage
#1076
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpsievert
reviewed
Jul 12, 2024
@@ -90,6 +90,7 @@ card <- function( | |||
|
|||
attribs <- args[nzchar(argnames)] | |||
children <- as_card_items(args[!nzchar(argnames)], wrapper = wrapper) | |||
children <- card_image_add_classes(children) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's investigate what this would mean for navset_card_*()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this works perfectly with navset_card_*()
.
Kapture.2024-07-15.at.10.30.52.mp4
cpsievert
reviewed
Jul 12, 2024
cpsievert
approved these changes
Jul 12, 2024
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the
card_image()
API and fixes a few minor rough edges, including fixing #1071.Currently
card_image()
is by default wrapped in acard_body()
container. This happens even if settingcontainer = NULL
because thecard_image()
result doesn't have acard_item
class. This makes it impossible to usecard_image()
for the card image caps described in the Boostrap docs.With this PR, we now default to
container = NULL
and, when this is the case, callas.card_item()
on theimage
so that the image is treated as a card item.I'm not sure how much value
container
adds -- it's easy enough to docard_body(card_image())
if that's really desired -- and I don't think there's any harm in unconditionally adding thecard_item
class to theimage
. I'd be in favor of deprecating thecontainer
argument (unless I've missed an important use case).border_radius
gains"auto"
, the new default value. Whenborder_radius = "auto"
, we add acard_image_auto
class to thecard_image()
output that is detected bycard()
. We then automatically add thecard-img-top
,card-img-bottom
, orcard-img
class if the card image is the first, last or only child. In all other cases, we don't modify the image tag.I changed the default value of
card_image(fill = )
toFALSE
.fill = TRUE
might have worked better whencard_image()
was always wrapped in acard_body()
but in most cases havingfill = TRUE
will cause the image to stretch, distorting the image. I don't think this is what most people want fromcard_image()
.I added a bit of logic such that a URI or non-local file path can be given to
file
. Instead of having to writeusers can just give the URL to
file
card_image("https://example.com/cat.png")
This is backwards compatible in the sense that when a value is passed to
src
, that value is used directly. The advantage is that we won't even consultefile
whensrc
is provided, allowingcard_image(src = "cat.png")
to skip the base64 encoding.I added
alt = ""
as a default argument that appears before the dots. This has the side effect of marking mostcard_image()
uses as decorative. That's probably better than the default of not having alt text at all, and it intends to bring increased attention to alt text without breaking existing apps by makingalt
required. Fixescard_image()
should include explicitalt
argument #1071Example app