Image placeholder styling #64
Conversation
|
And what's the current result? |
theme/icons/image_placeholder.svg
Outdated
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 700 250"> |
There was a problem hiding this comment.
From the ticket description:
We should also create placeholder that have different width/height ratio to not take as much vertical space when it will be on 100% of the width.
There was a problem hiding this comment.
I will change the file to optimized one.
src/imageuploadprogress.js
Outdated
| @@ -73,7 +73,7 @@ export default class ImageUploadProgress extends Plugin { | |||
|
|
|||
| // Show placeholder with infinite progress bar on the top while image is read from disk. | |||
| if ( status == 'reading' ) { | |||
| viewFigure.addClass( 'ck-appear', 'ck-infinite-progress' ); | |||
| viewFigure.addClass( 'ck-appear', 'ck-infinite-progress', 'ck-placeholder' ); | |||
There was a problem hiding this comment.
I can see now how this class is used, but I got confused because we already use ck-placeholder but in a different case.
I think that what's wrong here is that those class names don't use BEM convention. E.g. this is image placeholder so it should contain some "image" part. Am I right, @oleq?
There was a problem hiding this comment.
This class is added to make sure that placeholder is always 100% width when is visible.
There was a problem hiding this comment.
It should be ck-image-upload-placeholder (if there's no ck-image-upload parent element).
What does ck-infinite-progress do? It should rather be ck-image-upload-progress_infinite (as opposed to hypothetical ck-image-upload-progress_finite). Or, if the "progress" is the name of the modifier, then ck-image-upload_progress_infinite.
ck-appear doesn't feel good either. It also belongs to the image upload ecosystem, so it should be at least ck-image-upload_appear.
We've got a guide for this https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style-Naming-Guidelines#css-classes.
There was a problem hiding this comment.
I can see now how this class is used, but I got confused because we already use ck-placeholder but in a different case.
I agree that this class should be named better. I totally forgot about it's other usage.
There was a problem hiding this comment.
I will use ck-image-upload-placeholder as @oleq recommends it. I will create separate ticket to clear the class naming mess.
|
This is how placeholder looks now on wider editors: vs
|
|
I think the "Uploading Image" text should be rendered with a non-serif font, like the rest of the UI – #7E8A97 text color is enough to match WCAG AA for large text. |
|
The color of the text also fails in terms of a11y standards:
|
|
Off-topic: I wish one day we can implement an UI similar to my mockups in https://github.com/ckeditor/ckeditor5-image/issues/16#issuecomment-279640113. |
|
Fixed font and contrast: |
Suggested merge commit message (convention)
Other: Placeholder image looks better on wider editors. Closes ckeditor/ckeditor5#2827 .