#21785 closed task (blessed) (fixed)
Add header image uploads with cropping to the customizer
Reported by: | nacin | Owned by: | |
---|---|---|---|
Milestone: | 3.9 | Priority: | normal |
Severity: | normal | Version: | 3.4 |
Component: | Customize | Keywords: | has-patch |
Focuses: | javascript, administration | Cc: |
Description
See #21355 for an explanation for why header image uploads (sans cropping) was removed from the 3.4 branch. This ticket is about adding it back, with a crop step, to ensure proper support.
Attachments (31)
Change History (123)
#3
@
12 years ago
I was looking at the code on the Custom Header, the usage of steps
on this page feels wrong;
We have the power to put all this stuff at the Media Box, in a way that the user have the same experience of cropping that he would have if he was inserting an image to the post.
Even if this is not the approach we take, at least the steps
should have a POST then GET, at every step, on the second step you get a duplicate of your image if you reload the page, then in the third step you will get a duplicated crop.
I would love to see this page rebuild from scratch, maintaining backwards compatibility but still rebuilt.
Not saying that this should be on 3.5 but...
#6
@
12 years ago
- Keywords 3.6-early added
- Milestone changed from 3.5 to Future Release
- Type changed from defect (bug) to task (blessed)
#12
@
11 years ago
- Focuses ui added
- Keywords 3.6-early removed
Perhaps the work around adding image editing to the media modal in 3.9 could be extended to fix this too. This bug has been around for far too long, and blocks #25571.
We should use the media modal for the entire upload process so that users can select existing images from the media library or upload a new file, before moving to a crop step, along the same lines as #21483.
#13
follow-up:
↓ 14
@
11 years ago
- Focuses javascript added; ui removed
Removing ui focus here, too; adding javascript. Yes, it affects UI, but that's not really the focus here.
#14
in reply to:
↑ 13
@
11 years ago
Replying to helen:
Removing ui focus here, too; adding javascript. Yes, it affects UI, but that's not really the focus here.
Actually, wouldn't both make sense, since we need to design a new UI and it'll be implemented with some JS wizardry (presumably in the media modal)?
Also, per something that came up in #21483, we might want to consider moving the "uploaded headers" section into the modal as well, if that's how this is implemented. Alternatively something inline could work, but given the width of the customizer controls that could be tricky. Ideally users could select images from the media library as well, but the cropping implementation approach will determine whether that gets fixed in this ticket.
Unfortunately, too much has changed since this was first opened for there to be a clear assumption of how the workflow for uploading/selecting and cropping headers in the customizer should work; if anyone has suggestions that might be a good first step toward finally making this happen.
#16
@
11 years ago
@ehg and @miguelcsf are working on it in #26505. We just talked about the current status two weeks ago, as a matter of fact, and they will spend the coming week on getting the .com implementation ready for core (I've been told).
#17
follow-up:
↓ 18
@
11 years ago
What Konstantin said. It currently works as a core plugin, although @ehg and I need to sort out the odd bug or two, as well as decouple it from our cropping extension of the Media Manager.
#18
in reply to:
↑ 17
@
11 years ago
Replying to mcsf:
What Konstantin said. It currently works as a core plugin, although @ehg and I need to sort out the odd bug or two, as well as decouple it from our cropping extension of the Media Manager.
If it can be done in the next week or so, I think this would be fantastic to add to 3.9.
This ticket was mentioned in IRC in #wordpress-dev by mcsf. View the logs.
11 years ago
#21
@
11 years ago
These patches add cropping support to a newly developed Header Image control in the Customizer. This is our first pass at merging the plugin we developed into core, we’re sure there are things to be added/corrected/debated — feedback would be greatly appreciated.
— Brought to you by team @ehg & @mcsf
- Adds a Cropper controller and view to the Media Manager + L10n
- Allows us to add HTML to the title/minimised menu item title
- Replaces
WP_Customize_Header_Image_Control
from scratch using:- Underscore templates;
- Extended backend logic to keep track of and remove recently used image.
- Adds a
wp.customize.HeaderControl
, which leverages:- The new cropping feature in the Media Manager;
- The existing
imgAreaSelect
plugin to select an area of the image to crop; - Backbone views, models and collections for header images.
- Refactors
wp-admin/custom-header.php
and adds 3 AJAX endpoints to it for adding, removing and cropping images. - Adds a lib,
jquery-slimscroll
, which we use to scroll uploaded header images in a limited height container. - Adds relevant CSS and a PNG image (see below).
- Adds
sinon.js
for mocking/stubbing/spying for qunit js tests. - Adds tests & fixtures for the Backbone header models/collections.
- Adds phpunit tests for header dimension calculations & extracted
custom-header.php
functions.
src/wp-admin/images/close-button.light.png
- The remove image button for the header control.
#22
@
11 years ago
Updated patches 21785-media-manager.2.diff, 21785-customize-header.2.diff and 21785-tests.2.diff address feedback from @nacin and @ocean90, namely:
- Replaced image (the "close" PNG) with Dashicon
- Ditched slimScroll
- AJAX: Added missing capabilities check
- AJAX: action adds alternatively use
$this
and__CLASS__
- L10n: Added missing l10n for "No image set" placeholder
- CSS: Removed wpcom-ism (
body:not(.blue)
)
- JS: Moved Header views and models from
wp-admin
towp-includes
- JS: Ditched the template in
wp.media.view.Cropper
- PHP: In
WP_Customize_Header_Image_Control::render_content
: braces around if and else blocks. - PHP: Use
wp_die
overdie
- Tests: have test classes extend
WP_UnitTestCase
- Tests: Move JS test fixtures to
index.html
- Check that JSHint rules are followed
- (not from feedback) Cleanup: Removed leftover parent attachment logic from previous feature
For reference, we're doing development at github:ehg/wordpress-develop; use /commits/dev?author=AUTHOR
with AUTHOR
in (ehg|mcsf)
for granular commits.
This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.
11 years ago
#24
@
11 years ago
I spent some time playing around with this and looking at the code and fixed up the following small issues in 21785-03.patch.
- Unbound the resize event handler when the Cropper view is removed
- Extend
WP_Customize_Image_Control
instead ofWP_Customize_Control
so properties such asget_url
are declared before the class is instantiated. - Switched the "Skip Cropping" to using the secondary button style instead of primary.
- Cleaned up some whitespace issues.
For fun, I also pushed these changes to a branch on Github: https://github.com/gcorne/develop.wordpress/tree/trac-21785-03
One thing that I didn't fix that I think we should address is how the new ajax handlers work. Instead of sending a string when an error occurs, we should be using wp_json_send_error
and wp_json_send_success
. Right now, if there is an error when cropping, JSON.parse()
throws an error and the modal is stuck until the user closes it.
I also wonder if we should change the terminology used for the buttons in the customizer. "Hide" and "Add new" don't feel quite right. I was thinking maybe it should be "Add" when there isn't a header image set, "Remove" and "Replace" when there is a single image set, and "Remove" and "Add" when the header image is set to randomize.
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
#28
@
11 years ago
21785-04.patch refreshes the patch.
While testing, I realized that it would be nice to cancel the cropping step and go back to the browser to select a different image. Add some sort of cancel, might be something worth adding in a future iteration.
#29
@
11 years ago
@gcorne thanks for this, and sorry for the delay!
We were actually looking into wp_json_send_*, but were unsure on how to present an error to the user. Any ideas?
Also, we originally had “Remove” in place of “Hide”, but we got some beta user feedback that “Remove” could be mistaken for “Remove image from media manager”. As for “Replace/Add” — we haven’t encountered any feedback that implies confusion over our existing “Add new” button so far, with just under 2 million cropped images since December — but we could be wrong. ;)
#30
@
11 years ago
- Focuses administration added
- Keywords has-patch added
This looks good! With testing so far, functions as advertised.
As I was reading through, went ahead and added a bit of code cleanup (in the patch attached) -- mostly brackets -- to what was added, and a couple of the functions changes were made in.
Before release, also could use hook docs.
#31
@
11 years ago
21785-06.patch is a refresh of 21785-05.patch.
#34
@
11 years ago
I love this new feature, exactly the easy workflow with a already set aspect ratio set and the user just postion and resize how they want it to be cropped.
But I really think it would be even better to also use it with featured images on pages. Then it would be so simple to get them cropped correctly which today is a bit complicated.
Maybe that is already possible to implement, maybe there are already plugins for this and maybe this is totally the wrong forum for this kind of comment...
#35
@
11 years ago
This is great. Really, I loved it, and it was so well done, too. In [27497], I made a number of changes beyond gcorne's latest patch. Most of them leave things to do:
- The "Randomize" and "Randomizing" strings were not translatable, as they substituted words in. We always need to be translating full sentences. Additionally, it didn't handle plurals properly, and we don't have the ability to do plurals in JS, so it was removed for the moment. Finally, TODO: we shouldn't show "Randomize" or "Randomizing" when there is only one image — you can't "randomize" one image.
- I added a
final
to the WP_Customize_Header_Image_Control class. This class changed significant shape and I wanted anyone who was extending it to get an immediate fatal error in 3.9. TODO: Investigate usage and backwards compatibility concerns. It's possible we just create a new class and empty this one, making it essentially an alias for WP_Customize_Image_Control, which it extends.
- "Suggested dimensions" was removed as it was in the wrong place. This should be placed elsewhere, probably above "Attachment Details". TODO: Restore "Suggested dimensions", remove Title/Caption/etc. Consider removing "Edit Image", which doesn't make sense here.
- I changed the XHR stuff to use wp.ajax.post and wp_send_json_*. TODO: All XHR requests need some kind of error handling, ideally. (There are existing tickets about this; we don't handle errors very well in the customizer currently, but there's a lot more things that can fail here.) I also removed the privilege-checking method from the main class, as it was an over-abstraction and hid important security checks under the rug.
- I just noticed "this string must not be empty" and I'm not sure what's going on there. TODO: Most of that logic should not be in the customizer control, but in the custom header class, so we'll have to figure that out.
- The dice just isn't clear that it's a dice, not at the font size size I see it in, anyway. (It looks like a broken unicode character.) TODO: We should use a dashicon or nothing here. (It also wasn't rotating for me — is it supposed to?)
- TODO: doCrop et. al should be moved to the models, and out of the views.
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
#37
@
11 years ago
Nice. ehg and I will work on some of those items. Meanwhile:
TODO: we shouldn't show "Randomize" or "Randomizing" when there is only one image — you can't "randomize" one image.
This has been raised in wpcom and is intentional. The idea was to make the feature self-discoverable—or, rather, self-advertising. It has also been suggested we change the label on the button to something like “Add more images to randomize”, but that would be i18n hell and introduce needless complexity.
I just noticed "this string must not be empty" [...] I'm not sure what's going on there
This is because of #23268, which we found through the Codex: http://codex.wordpress.org/Class_Reference/WP_Query#Custom_Field_Parameters
The dice just isn't [...]
It’s just supposed to gradually change color when you hover over the button. ;) Agreed, though, that we should use a Dashicon. Making it roll would be cool!
#38
follow-up:
↓ 41
@
11 years ago
TODO: Investigate usage and backwards compatibility concerns of WP_Customize_Header_Image_Control class.
I’ll get back to you soon on this - preliminary findings indicate that no one is using the class, but we want to search all plugins and themes for any usage of WP_Customize_Header_Image_Control. I’m currently checking out the plugins and themes repos to search for any mention of the class - opengrok for these repos would be useful, as the repos are rather large!
TODO: Restore "Suggested dimensions", remove Title/Caption/etc. Consider removing "Edit Image", which doesn't make sense here.
Placing the suggested dimensions in the Attachment Details sidebar feels a bit wrong, as the suggested dimensions aren’t particular to a specific image (see https://cloudup.com/iTMNezhnAI6). The idea behind putting them in the Media Manager’s title was to show the dimensions no matter what context you’re in - the suggested dimensions can be seen when you’re uploading *and* selecting an image (see https://cloudup.com/iDXX9n9Busq). An alternative would be to just not show the dimensions in the Media Manager at all, and rely on them being noticed in the Header section.
TODO: All XHR requests need some kind of error handling, ideally.
We’d like to make the case for a better, broader solution to error handling that would go beyond this ticket: adding a Customizer error handler that would display error messages by binding to customize(‘error’) events - see https://cloudup.com/ihRG2lmDTkC . We could also use the proposed error handling in the Customizer to show ‘crop failed’ events from the Media Manager.
TODO: Most of that logic should not be in the customizer control, but in the custom header class, so we'll have to figure that out.
See attachment:21785-move-query-logic.diff. @mcsf “took some liberty” and made sure process_default_headers
could only run once. If this is wrong, let us know.
TODO: We should use a dashicon or nothing here.
@melchoyce is on the case :)
TODO: doCrop et. al should be moved to the models, and out of the views.
See attachment:21785-doCrop-move.diff
Aside:
@nacin - was there any reason why the unit tests weren’t committed? Do they need more work?
#40
in reply to:
↑ description
@
11 years ago
Included attachment:21785-fix-randomize.diff which fixes an issue introduced in [27497] where:
- The labels for the "Randomize" buttons (as well as the "Randomizing" label in the Current Header preview) would not show, thus making the color-changing dice a very lonely one
- The overlay close button on the corner of each user-uploaded image would not show
I believe that, following some translation-related fixes, nacin wanted to remove the "nImage" property instead of "type" in wp.customize.HeaderTool.ChoiceView.prototype.extendedModel. A delete-typo, if you will.
#41
in reply to:
↑ 38
;
follow-up:
↓ 47
@
11 years ago
Replying to ehg:
Placing the suggested dimensions in the Attachment Details sidebar feels a bit wrong, as the suggested dimensions aren’t particular to a specific image (see https://cloudup.com/iTMNezhnAI6). The idea behind putting them in the Media Manager’s title was to show the dimensions no matter what context you’re in - the suggested dimensions can be seen when you’re uploading *and* selecting an image (see https://cloudup.com/iDXX9n9Busq). An alternative would be to just not show the dimensions in the Media Manager at all, and rely on them being noticed in the Header section.
I get why you'd want them to be shown during upload — at that point, perhaps it would be appropriate to include it actually in the uploader instructions. As in, near "Maximum upload file size:", where allowed file types would be listed, etc.
My point was that the suggested dimensions should appear *above* the Attachment Details sidebar, such as how gallery settings are rendered. Likewise, we don't want/need captions or other fields here.
We’d like to make the case for a better, broader solution to error handling that would go beyond this ticket: adding a Customizer error handler that would display error messages by binding to customize(‘error’) events - see https://cloudup.com/ihRG2lmDTkC . We could also use the proposed error handling in the Customizer to show ‘crop failed’ events from the Media Manager.
Yeah, I think that makes sense. For the moment, we just need to make sure that if/when an XHR fails, things don't go completely haywire. (No JS failures, etc.)
was there any reason why the unit tests weren’t committed? Do they need more work?
Omitting them wasn't intentional. I'll get them in. Was there a decision on sinon?
#44
@
11 years ago
attachment:21785-dashicons-randomize.diff makes use of the new Randomize dashicon (added in [27705])
#45
@
11 years ago
attachment:21785-doCrop-move.2.diff - updated doCrop -> Attachment model refactor as per gcorne and nacin's feedback.
Also, I was wondering whether there's a better way to get the lastState's selection, rather than doing
this.set('selection', new media.model.Selection(this.frame._selection.single));
#46
@
11 years ago
attachment:21785-suggested-dimensions.diff is a WIP patch that replaces the sidebar attachment details with a theme suggested dimensions view.
I'm still not sure if it's worth doing this, as it's a significant patch for what I perceive as little value, with a bit of a weird UX. However, I could be Doing It Wrong™ :)
In the patch:
- New media.view.Attachment.SuggestedDimensions that renders a new crop-suggested-dimensions template, which is based on a cut down attachment-details template.
- Render the SuggestedDimensions view if crop options are supplied to wp.media(). I'm setting the cropOptions in the attachment model here, which feels wrong.
#47
in reply to:
↑ 41
@
11 years ago
Replying to nacin:
We’d like to make the case for a better, broader solution to error handling that would go beyond this ticket: adding a Customizer error handler that would display error messages by binding to customize(‘error’) events - see https://cloudup.com/ihRG2lmDTkC . We could also use the proposed error handling in the Customizer to show ‘crop failed’ events from the Media Manager.
Yeah, I think that makes sense. For the moment, we just need to make sure that if/when an XHR fails, things don't go completely haywire. (No JS failures, etc.)
Apart from the Media Manager's "Cropping..." button being stuck on an error, the two other XHR requests (header_add/header_remove) will just silently fail on errors.
attachment:21785-mm-crop-error-button.diff (depends on attachment:21785-doCrop-move.2.diff) changes the "Crop" button to "Error? Retry" on an error - pic: https://cloudup.com/cb4uy8jfehO
The button in its error state is clickable, so users can retry. I'm assuming most errors will be network/server related - so this could be a reasonable approach.
This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
#54
follow-ups:
↓ 55
↓ 61
@
11 years ago
- 21785-doCrop-move.2.diff — Leaving this uncommitted pending a review from gcorne. He indicated to me it would make sense if doCrop then refreshed the model's attributes; there is also a question in comment:45 about referencing a selection.
- 21785-suggested-dimensions.diff — I agree, mhg, that this is not ideal. Sorry for sending you on that goose chase. One of the reasons the original suggested dimensions didn't get committed is that it had this pretty weird hack with regards to the media manager title. I'm fine with the design, but I'd want to inject that view in another way.
- 21785-mm-crop-error-button.diff — Nice! Chances are a crop isn't a transient failure, so I'm not sure how much a retry will help, but we have very little recourse here. Asking others to weigh in to see if anyone else can think of an existing paradigm we should otherwise copy.
- 21785-move-query-logic.diff + consolidated-query-logic-1.diff looks good. I took that a step further in [27849] and had the get_uploaded_header_images() method wrap the get_uploaded_header_images() function — nice and clean!
- I'm curious about whether get_default_header_images() needs to do all of that work. A default image is not allowed to be selected on Appearance > Header unless it is also a default header. I don't want to condone the practice of registering a default image *and* other default headers *and* not specifying the former with the latter. It doesn't work now, I'd like to leave it that way. It would still appear as the "Current Header" but wouldn't show up in "Suggested". If the issue is there is no "Restore Original" button, then OK, I can see that.
- I have been using the UI more and more over the last few days and I am going to try to bring back the tabs and see how they work. I discussed this a bit in IRC mostly as it relates to "Randomize" and "Randomizing" being too dark and not really feeling like buttons (like one of them is) or a current state (like the other is). That's not a big deal, but I'm also concerned about all of the jumping around that occurs. It's very jarring. If you click "Randomize" things jump up top. If you click a suggested header when you already have a suggested header, the one you clicked suddenly vanishes and another takes its place. Sometimes you get jumped up (on purpose), but that's not a good experience either. I'm going to try to re-implement the tabs with some kind of a checkbox or link for Randomize to see if it feels better.
#55
in reply to:
↑ 54
;
follow-up:
↓ 57
@
11 years ago
Replying to nacin:
- 21785-mm-crop-error-button.diff — Nice! Chances are a crop isn't a transient failure, so I'm not sure how much a retry will help, but we have very little recourse here. Asking others to weigh in to see if anyone else can think of an existing paradigm we should otherwise copy.
I think it should be more like these errors: https://cloudup.com/c4LTYurtj7z
The image should be replaced by the error message with a link to retry. The Crop button should be disabled when an error exists.
#56
@
11 years ago
The error ocean90 references can also appear in the sidebar, if you're on the library view.
#57
in reply to:
↑ 55
@
11 years ago
Replying to ocean90:
Replying to nacin:
- 21785-mm-crop-error-button.diff — Nice! Chances are a crop isn't a transient failure, so I'm not sure how much a retry will help, but we have very little recourse here. Asking others to weigh in to see if anyone else can think of an existing paradigm we should otherwise copy.
I think it should be more like these errors: https://cloudup.com/c4LTYurtj7z
The image should be replaced by the error message with a link to retry. The Crop button should be disabled when an error exists.
attachment:21785-mm-crop-error-overlay.diff overlays the error message on top of the image. I avoided a 'retry' link, to keep the patch minimal.
Pic: https://cloudup.com/cb02wO5skg0
It:
- Uses a template for Cropper view, rather having it as an img element, so we can add more elements to it in a clean way.
- Re-uses media.view.UploaderStatusError to overlay cropping errors onto the image
- Fixes an issue where imgAreaSelect was referencing the wrong parent element - which lead to weirdness with z-indexes in the crop-content template.
#58
@
11 years ago
attachment:21785-accessibility.diff implements accessibility suggestions from #27598
- Convert Add/Hide <a> buttons to <button> buttons
- Rename "Hide" to "Hide image" and "Add new" to "Add new image"
- Convert image <a>s into <button>s and add screen reader text
- Use alternate text meta data for uploaded image alt text
- Use suggested header names for suggested image alt text
#59
follow-ups:
↓ 60
↓ 64
@
11 years ago
attachment:a11y-ui-improvements.diff includes ehgy's accessibility changes (see above), as well as UI improvements as discussed in IRC, namely:
- Prefer a lighter UI.
- Make the UI more coherent with the rest of the Customizer (button style, borders).
- Don't hide the selected header, but rather highlight it. Adjust logic accordingly.
- Stop the Customizer controls from jumping around when selecting a header.
Plus:
- Fix ordering of headers by defaulting timestamps to 0.
- Force height for Randomize buttons instead of inferring it from other Header views.
- Some removal of code.
#60
in reply to:
↑ 59
@
11 years ago
Thanks for the quick patches ehg and mcsf! I'll test this and anything else added from an accessibility perspective tomorrow, and provide any feedback.
#61
in reply to:
↑ 54
@
11 years ago
Replying to nacin:
- 21785-suggested-dimensions.diff — I agree, mhg, that this is not ideal. Sorry for sending you on that goose chase. One of the reasons the original suggested dimensions didn't get committed is that it had this pretty weird hack with regards to the media manager title. I'm fine with the design, but I'd want to inject that view in another way.
Yep, the hack was necessary because the title isn't easily as easily editable as the rest of the MM views, such as the toolbar and content. We could turn the titlebar itself into a region, to give us cleaner code and more flexibility in the future - but this would involve significant changes, which probably aren't appropriate at this time in the release cycle.
Should we just rely on the suggested dimensions being in the Header control for now, and revisit this for a later release?
This ticket was mentioned in IRC in #wordpress-ui by davidakennedy. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.
11 years ago
#64
in reply to:
↑ 59
@
11 years ago
Replying to mcsf:
attachment:a11y-ui-improvements.diff includes ehgy's accessibility changes (see above), as well as UI improvements as discussed in IRC, namely:
- Prefer a lighter UI.
- Make the UI more coherent with the rest of the Customizer (button style, borders).
- Don't hide the selected header, but rather highlight it. Adjust logic accordingly.
- Stop the Customizer controls from jumping around when selecting a header.
Plus:
- Fix ordering of headers by defaulting timestamps to 0.
- Force height for Randomize buttons instead of inferring it from other Header views.
- Some removal of code.
See #27598.
Regarding first issue: The current buttons in the new header image area of the theme customizer are not actual buttons... This works perfectly now and provides much more context to assistive technologies.
Second issue: This type of pattern is even more problematic in the "Randomize uploaded headers" button... This works perfectly now and provides much more context to assistive technologies. However, it looks like there's something off visually for me. This may be an on my end... See the button-like style happening behind each randomized button: https://www.dropbox.com/lightbox/home/Public/WPTicket-27598
Third issue: Also the markup for an uploaded header image that can be clicked to be set needs more context as well... This also works perfectly now and provides much more context to assistive technologies.
Fourth issue: If a user explicitly sets alternative text in the Media Library, that attribute doesn't seem to be displayed in the Theme Customizer Header Image area... The Header Image for Current image does not seem to be getting an alternative text attribute. The other two (Previously uploaded and Suggested) do, and that's great! The Current image header does need one in order for screen reader users to know which image is there. This would be a must have.
Great work ehg, mcsf and others! This also sets us up well for future enhancements here, especially around WAI-ARIA.
Let me know if there are any questions!
#65
@
11 years ago
attachment:a11y-ui-improvements-2.diff is a refresh of attachment:a11y-ui-improvements.diff that addresses some of davidakennedy's latest feedback (thank you!):
- Add image caption/description to current image's alt text
- Add some tabindices to possibly help screenreader navigation
It also includes UI tweaks as discussed on irc:
- Don't bump headers on select
- Place border over selected header without shrinking it
Props ehg
#66
@
11 years ago
attachment:a11y-ui-improvements-3.diff is a refresh of attachment:a11y-ui-improvements-2.diff that fixes an outline issue within the Randomize button.
This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.
11 years ago
#68
@
11 years ago
attachment:21785-suggested-dimensions.4.diff adds suggested image dimensions to the attachments browser and uploader views.
In the patch:
- Remove old suggested code
- Add a crop object to the frame options, which contains the suggested dimensions as well as the imgAreaSelect options
- Change references to imgSelectOptions
- Display the suggested dimensions if crop is defined in the frame options
- Add dimensions to uploader view
Pics:
https://cloudup.com/c5M3XF8YtE6 & https://cloudup.com/cKxy1a0UDJC
#71
follow-up:
↓ 74
@
11 years ago
21785.suggested-dimensions.5.diff seems fine. I want gcorne to look at this as it changes media API.
What's left here? Anything that still needs to be committed? Anything that still needs to be done?
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
#73
@
11 years ago
attachment:21785-a11y-ui-improvements.2.diff fixes the outline issue with the Randomize buttons. We're now directly styling the button.
#74
in reply to:
↑ 71
@
11 years ago
Replying to nacin:
21785.suggested-dimensions.5.diff seems fine. I want gcorne to look at this as it changes media API.
What's left here? Anything that still needs to be committed? Anything that still needs to be done?
To be committed:
- attachment:21785-doCrop-move.2.diff (possibly needs review for selection reference)
- attachment:21785-a11y-ui-improvements.2.diff
I don't think there's anything else outstanding.
#75
@
11 years ago
I performed a quick retest, and things hold up well. A few points of feedback:
- The tabindex attributes added here are not needed: https://github.com/ehg/wordpress-develop/commit/87c993635cff79e237732a61b76c06697d620251#commitcomment-5916062 Is there a particular reason why they might be needed I'm missing? The source order here is very logical and everything can be tabbed to naturally so no need for the tabindex.
- Everything still performs nicely with a screen reader.
- The buttons that are images do not have the best visual indicator on focus. We've made some improvements with #27173. Can we add some kind of visual indicator here when those elements receive focus?
#78
@
11 years ago
21785-doCrop-move.3.diff has one small change from 21785-doCrop-move.2.diff. There was one place in createCropperContent
where .first()
instead of .single()
was being called. I think that this latest patch is ready to be committed.
#79
@
11 years ago
I spent some time looking over 21785.suggested-dimensions.5.diff and started to wonder if it made sense to make suggestedWidth
and suggestedHeight
a bit more general. I also felt like for at the very least consistency's sake that it made sense for the data to be kept with the state instead of the media frame. This thought process led to 21785.suggested-dimensions.6.diff, which also adds a space between "Suggested image dimensions:" and the actual dimensions in the attachment browser view.
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
#83
@
11 years ago
- Resolution set to fixed
- Status changed from new to closed
Wow! Done! Please open new tickets for new issues/improvements.
I'm excited about this. This was a lot of work and it's really paid off — this is a great feature we're shipping! Thank you so much mcsf and ehg for your big contribution. :-)
I went through 21785-doCrop-move.2.diff with gcorne in IRC and we decided not to make the change. Feel free to re-open or comment if you disagree (or agree).
Related: #21820