-
Notifications
You must be signed in to change notification settings - Fork 3
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
Updated and expanded feature images #1430
Conversation
🦋 Changeset detectedLatest commit: 0aba263 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@@ -0,0 +1,5 @@ | |||
--- | |||
'@cloudfour/patterns': minor |
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.
Is a changed file path for an asset a breaking change? I guess so, but since we weren't using it yet, and since it wasn't a code change, I chose minor
. Second-guessing that now.
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.
I'm fine either way 👍
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.
These are looking great! I left one question about alt text inline.
Out of curiosity, when would you want to use the blank version over the Cloud Four version? Is that meant to be a starting point for creating new designs? Or is it meant to be used directly on the Wordpress site?
{% block content %} | ||
{% for image in images %} | ||
<figure> | ||
<img src="{{image.src}}" alt=""> |
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.
Should we include alt text for these images? We'll want to when we use them on the Wordpress site. Should the pattern library define and expose those alt descriptions?
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.
Ahh I see we're just pulling the name from the filename. I was imagining there was a JSON file somewhere of data. Hmm, what do you think? Is there a good place to define those alt descriptions in the patterns?
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.
The WordPress site will have a lot more information about the article the image is supposed to represent, so I'm not sure that specifying alt
text here will be very useful. I think it may actually complicate implementation.
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.
Cool, that works for me 👍
It might be either! To be honest, I'm still figuring it out on the WordPress side of things. But it seemed potentially useful. |
Overview
This PR makes a number of updates to our fallback feature images:
blank
andcloudfour
as potential fallbacks. These will be used by the theme when an appropriate image cannot be found.Canvas
elements from this page because the code samples felt irrelevant, it feels more like a design reference to me.Screenshots
Testing
Deploy preview
/CC @AriannaChau