Conversation
|
* div to paragraph tag to prevent hiding from JS which targets sibling divs * special handling for displaying the before description before the UL element in tabbed groups
astratagem
left a comment
There was a problem hiding this comment.
Looks good to me overall! I have one blocking question about the use of ID selectors in one place, and some suggestions about using more prominent section divider comments in CSS.
js/media/fieldmanager-media.js
Outdated
| } else { | ||
| var preview = 'Uploaded file: '; | ||
| preview += wp.media.string.link( props ); | ||
| preview += '<a href="#"><span class="dashicons dashicons-media-document"></span></a>'; |
There was a problem hiding this comment.
Is there a risk of these media icon classes becoming out of sync with the filtered icon class used in PHP?
There was a problem hiding this comment.
@dlh01 I think the filter may not have a solid use case. I pushed up a change that will set the icon depending on the attachment icon type. See https://github.com/alleyinteractive/wordpress-fieldmanager/pull/468/files#diff-7f0fb55a63140bb4d44360770c1ec06ff18be39f6c9e359f8f7370bca480efcbR81-R83
Let me know what you think about his change.
There was a problem hiding this comment.
We are now relying on the media type from the attachment to output the correct icon.
|
Couple potential edge cases I noticed while testing this branch on a client site: Checkbox with a long label in the sidebar of the block editor breaks to a new line: Checkboxes within a tabbed group start to the left of the tab (not actually sure whether this is a defect): These cases aside, plus the few notes inline, the branch is looking really good to me. I'm excited to get it merged! Let me know what you think about the comments and how it'd be best to handle them, including in a follow-up PR. |
Co-authored-by: David Herrera <david@alley.co>
|
@stevenslack Re. 9dd2cc2, where does the class |
|
@dlh01 The wrapper element, The issue you are referring to where a checkbox with a long label in the sidebar of the block editor is present on |
|
Interestingly, I see that an instance of As for the checkbox label, it looks as expected to me in Regardless, if you'd prefer to move the discussion to a separate issue, let me know. |
|
@dlh01 given the age on this PR I'd like to move the discussion of the checkbox label wrapping to another issue. |



Uh oh!
There was an error while loading. Please reload this page.