-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Image Blocks: Adding an image thumbnail selector #2435
Conversation
771bf55
to
2a5294a
Compare
Codecov Report
@@ Coverage Diff @@
## master #2435 +/- ##
==========================================
- Coverage 26.98% 26.87% -0.11%
==========================================
Files 159 160 +1
Lines 4903 4923 +20
Branches 817 820 +3
==========================================
Hits 1323 1323
- Misses 3035 3052 +17
- Partials 545 548 +3
Continue to review full report at Codecov.
|
Thank you for working on this! This implementation seems right on, though it does sort of present some questions around the idea itself, for which we might need some input by @karmatosed and/or @joemcgill. For example, what size image is inserted by default? Also should you be able to set a thumbnail to be full-wide? Noticed some issues with selecting the size in the dropdown, btw: |
2a5294a
to
5de15cd
Compare
Good catch @jasmussen The selected size issue should be fixed now. |
|
||
// Disable reason: A select with an onchange throws a warning |
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.
This isn't so much a reason as it is stating a fact. The rule exists because onChange
and onBlur
have accessibility impacts when used in select
elements. See also #2227
IIRC the current editor remembers this as a preference when making a selection. |
Still waiting for input for the next steps here: whether we should go this route or not? and if we should do anything about the block alignments when selecting a thumbnail size? |
Let's do this:
That will let us move forward, and revisit any issues that come up as a result of this. A future enhancement to potentially explore (perhaps with some srcset stuff in mind #869) would be to have some automated smarts in which size was picked. For example for wide and fullwide, pick full size. For resized images, perhaps pick a thumbnail. |
@karmatosed What logical order are you proposing? biggest size to smallest? Edit: Sorry, just saw the alphabetical order note :) |
Hmm maybe that works in reverse because we are setting to fullest? Lets go with biggest to smallest. |
@karmatosed Just pushed the alphabetical order :)
@jasmussen It's already the case for me. we're taking the default |
e73c442
to
605bf9d
Compare
That's cool! Unable to test the branch this time, but if you fixed the issue where the choice wasn't sticky (dropdown reverted) then 👍 👍 from me! |
closes #1727
This adds an inspector selector to pick the desired thumbnail size.