Closed IreneStr closed 5 years ago
I've discussed the constants with @andizer and decided to use constants for the dimensions (specific heights and widths), but not for the statuses (loading, loaded, errored) and modes (square, portrait, landscape). It's not likely that those status and mode names will ever change, and const SQUARE = square
doesn't add a useful abstraction.
I've added a default, empty, image alt
attribute. Without it, screen readers will try to read something from the image, often the image file name:
We should then revisit this when the implementation will be complete. Passing the alt attribute set by users will allow to set the og:image:alt
, see https://github.com/Yoast/yoast-components/issues/812#issuecomment-456851589. Also, in an authoring context, users need to know which image is set: when the image has no original alt attribute, we should use something like alt="The current image has no alternative text. The file name is: %s"
only in the preview (note: this is what WordPress does in the image widget). Not sure if we can get the file name though.
Also:
FacebookSiteNameWrapper
to a paragraph to be slightly more semantic: margins can be styled in the pluginImportant considerations:
1
I'm not sure I understand if there's the need to handle the initial state, when users haven't set an image yet. I see the image src
is required and the initial state is loading
. Maybe I'm missing some details.
2
Re: the layout: it's important to be aware the image is a flex item within a flex container, where the default value for align-items
is stretch
: this means images will be stretched by their height and this might affect the positioning / centering.
3 In Internet Explorer 11, the portrait image is not centered: I've tried to understand why, with no luck.
Maybe I've found a way to make Internet Explorer 11 happy. Will try it, then move back to Needs Action for feedback on the pending points.
Too many flexbox bug in IE11 to center the images. Finally switched to absolute positioning and
transform translate()
, which fixes the issue.
componentDidMount
Test with images that are "very landscape" and "very portrait", for example:
http://via.placeholder.com/1200x300
http://via.placeholder.com/300x800
When changing images, refresh the page as the image props are set only on mount.
Pending feedback on:
img.src = src;
in the promise is meant to doUncaught (in promise)
error in the console but couldn't reproduce consistently@afercia Thanks for your additions!
Some findings (in Chrome) based on your changes:
The example portrait image is not centered horizontally. The transform: translate( -50%,-50% );
doesn't do anything
I recall you discussing an alt image input field in the UX channel/with the architects. Did you reach a coonclusion about whether or not we'd want such a field?
The following will be addressed in a separate issue (which hasn't been created yet):
I'm not sure I understand if there's the need to handle the initial state, when users haven't set an image yet. I see the image
src
is required and the initial state isloading
. Maybe I'm missing some details.
We'll handle the following as well in that issue:
- depending on the final implementation, there might be the need to update the image properties when the image is updated. At the moment, they're set only on
componentDidMount
I've had the following error before I implemented this return in this catch: https://github.com/Yoast/yoast-components/pull/818/files#diff-cf84bc61f566d30a40d4c9261fb0306cR98
- on the "faulty image" example, I've got an
Uncaught (in promise)
error in the console but couldn't reproduce consistently
The example portrait image is not centered horizontally. The
transform: translate( -50%,-50% );
doesn't do anything
@IreneStr please try to refresh the page. It does center the image in all browsers for me.
I recall you discussing an alt image input field in the UX channel/with the architects. Did you reach a coonclusion about whether or not we'd want such a field?
Haven't. Anyways, we should make sure the final implementation renders an og:image:alt
meta in the markup in the front-end. It would be nice to have it also in the preview.
@afercia Refreshing, restarting the server, and clearing the cache all don't work for me, both in Chrome as well as Firefox 😕
@IreneStr Looking into it.
@IreneStr maybe not having a wrapper that centers the examples in the page doesn't help. The content is aligned to the left edge of the screen so noticing the image is actually centered is difficult. This is what I see when I add a wrapper to center the content:
Note: the CSS that first uses position: absolute; top: 50%; left: 50%;
and then translate()
is a standard technique to center content.
The image is centered now 👍
I think a final acceptance test by someone who didn't write a large part of the code would be in place, so I'm moving it to needs-acceptance
.
Acceptance 👍
Summary
This PR can be summarized in the following changelog entry:
FacebookImage
component.Relevant technical choices:
Test instructions
This PR can be tested by following these steps:
UI changes
Fixes #812