Image component prototype #318
Conversation
…o tk/image-component
acketon
left a comment
acketon
left a comment
There was a problem hiding this comment.
This is awesome @timkingbu , I really enjoyed going through it and I was able to compile it locally. The changes I suggested here should help get more of the functionality working in the sandbox-image block.
It looks to me like you have all of the hard parts figured out here and now it just needs some of the finishing touches, some UI tweaks, and some of the glue to make it so the data can actually be saved to the block.
I have a lot of little suggestions in here to make the demo block work better, fix a couple glitches, and then a bigger change to think about regarding how we should handle Alt Text. We can chat about it some next week too.
I'd wait to merge it though until it's a bit more functional as a working example.
👍
| "mediaId": { | ||
| "type": "string" | ||
| }, | ||
| "className": { |
There was a problem hiding this comment.
className should not be needed as an attribute. I believe we have some existing blocks that did this, it might have been needed in the past, but at least now I believe className is a property on the props and doesn't need to be added as an attribute and might cause conflicts.
If it ever does need to be added there is a way in the supports section to turn off className support.
| let altText = ''; | ||
| if ( altSource === 'alt' ) { | ||
| altText = imgObj.alt; | ||
| } else if ( altSource === 'caption' ) { | ||
| altText = imgObj.caption; | ||
| } else if ( altSource === 'title' ) { | ||
| altText = imgObj.title; | ||
| } else if ( altSource === 'description' ) { | ||
| altText = imgObj.description; | ||
| } else { | ||
| altText = altSource; | ||
| } |
There was a problem hiding this comment.
So for this I'd recommend splitting the altSource and the alt text. I'd add an alt attribute that is a string that holds the actual alt text saved in the block. Then altSource can just be set to alt, or caption, etc.
We could have a custom option as well.
In the InspectorControls have a TextControl for the alt attribute. If the imgObj has alt text from the media library we can use then the TextControl for alt can be disabled, but populated with that value.
Below that TextControl we also could let the user switch the altSource to custom and enable the TextControl field for the Alt attribute so they can edit and change the alt text...overriding what came from the media library.
I think this would have a few benefits:
- If the media library has an alt text value we can default to using that... helps get something on the images
- If the media library has no text we can use the
altTextControl is enabled and maybe even highlighted visually to indicate it has an empty value and must be filled out. - If we add a DecorativeImage toggle switch the user could identify it as an image that doesn't need alt text and we can handle that in an accessible way as well.
| <div> | ||
| { /* Show InspectorControls if the user is able to replace the image or change the focal point. */ } | ||
| { ( canEditImage || canEditFocalPoint ) && ( | ||
| <InspectorControls> |
There was a problem hiding this comment.
I'd suggest moving the InspectorControls to it's own partial as a component that can be imported into the main Image Component to keep this file cleaner
| */ | ||
| export const Image = ( props ) => { | ||
| const { | ||
| // Setup |
There was a problem hiding this comment.
We need to add some handlers for when the image is changed so the block can then save/update the attribute that stores the image id example: (empty function by default) Then when the select happens inside the component and we successfully have an image id we call whatever function was passed to onSelect
| // Setup | |
| // Setup | |
| onSelectImage = () => {}, | |
| onRemoveImage = () => {}, |
| tag = 'img', | ||
| size = 'thumbnail', | ||
| sourceSizes = [], | ||
| altSource = 'alt', |
There was a problem hiding this comment.
| altSource = 'alt', | |
| alt = '', | |
| altSource = 'alt', |
I'd recommend keeping the logic for the alt text value and the altSource separate to keep things simpler. In most cases in a real world block we'd want to just set the altSource to one value specific to the block, fetch the default alt text from that data in the media library for instance but still let the user edit the alt text from the block by showing a text field that would override it.
In fact it might be worth just building an alt text field into the Image component so we always have that supported in every block?
There was a problem hiding this comment.
Thinking about this more as I go through the code I'm even more inclined to lean toward adding some settings to the InspectorControls for alt text. We know our users don't do a good job with this and don't understand the rules. Adding a section of settings for Alt text gives us a chance to link to documentation, tell them it is required, etc.
We can also use a ToggleControl to handle the alt text for decorative images properly. We could have a toggle for "Decorative Image, no alt text" and if it is active set the alt attribute to "" so it outputs an empty alt="" so screen readers skip over it. See: https://accessibleweb.com/question-answer/when-should-i-use-a-null-or-empty-alt-tag/
| console.log( 'Image Media Fetch in Progress: ', isResolvingMedia ); | ||
| } | ||
| return ( | ||
| <LoadingSpinner |
There was a problem hiding this comment.
This LoadingSpinner is meant to be used overlaid over content... such as centered over something.
You could try setting shadow={false} on it (or maybe we should just make that the default)
Alternatively we can just use the core Spinner or perhaps load this positioned on top of some placeholder element that represents the image that is being loaded? Like a grayed out box or something...
| sourceSizes.forEach( function ( imgSource ) { | ||
| const imgSourceObj = getImageData( mediaObj, imgSource.srcset ); | ||
| if ( debug ) { | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'imgSource: ', imgSource ); | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'imgSourceObj: ', imgSourceObj ); | ||
| } | ||
| if ( imgSourceObj ) { | ||
| // for picture | ||
| sourcesTag.push( | ||
| <source | ||
| srcSet={ imgSourceObj.src } | ||
| media={ imgSource.media } | ||
| type={ imgObj.mime_type } | ||
| /> | ||
| ); | ||
| // for imgs | ||
| srcsetAttributeArray.push( | ||
| `${ imgSourceObj.src } ${ imgSource.descriptor }`.trim() | ||
| ); | ||
| } | ||
| } ); | ||
| srcsetAttribute = srcsetAttributeArray.join( ', ' ); | ||
| if ( debug ) { | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'sourcesTag: ', sourcesTag ); | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'srcsetAttribute: ', srcsetAttribute ); | ||
| } |
There was a problem hiding this comment.
Could some of this move into block-imports like getImageData()? Maybe a small component that returns a set of
Description
Moves the football on https://github.com/bu-ist/id-gutenberg/issues/176
Sandbox links
You can see my changes in my sandbox here.
There are really only 2 files to review here:
Does not address the following: