Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Added CSS upload loader #219

Merged
merged 19 commits into from
Jul 10, 2018
Merged

Added CSS upload loader #219

merged 19 commits into from
Jul 10, 2018

Conversation

oskarwrobel
Copy link
Contributor

@oskarwrobel oskarwrobel commented Jul 5, 2018

Suggested merge commit message (convention)

Feature: Added CSS upload loader. Closes ckeditor/ckeditor5#5150.


Additional information

Required ckeditor5-theme-lark/t/ckeditor5-image/207

Hardcoded text Uploading image... has been replaced by a nice looking CSS spinner.

SVG file is still used as an image source while the upload is in progress because:

  1. image can't have an empty source
  2. svg file sets the size of the image and this size is scalable (e.g. when switching image to side image)

Preview

upload-placeholder

@coveralls
Copy link

coveralls commented Jul 5, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 308c834 on t/207 into 30ea1c6 on master.

@oskarwrobel
Copy link
Contributor Author

@dkonopka

image
https://github.com/ckeditor/ckeditor5-image/pull/219/files#diff-1dd60618d87727a52cc7ebc65fbaa2eeR26

image

&::before {
content: '';
position: relative;
width: var(--ck-upload-placeholder-loader-size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Styles like width, height, belong to Lark. position, top, content, flex should stay here.

@@ -1 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 700 250"><g fill="none" fill-rule="evenodd"><rect width="700" height="250" fill="#FAFAFA" rx="4"/><text fill="#5F6F77" font-family="Arial,sans-serif" font-size="24"><tspan x="247.9" y="135">Uploading image…</tspan></text></g></svg>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 700 250"><g fill="#FAFAFA" fill-rule="evenodd"><rect width="700" height="250" rx="4"/></g></svg>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, why do we need that SVG? It's displaying a gray area, that's what it does? Couldn't we use a CSS element instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SVG file is still used as an image source while the upload is in progress because:

  1. image can't have an empty source
  2. svg file sets the size of the image and this size is scalable (e.g. when switching image to side image)

@oleq
Copy link
Member

oleq commented Jul 9, 2018

I can see 2 scenarios:

  1. I upload the image
    • the horizontal bar on the top is displaying the progress,
    • I can see the image immediately so there's no need to display the spinner
  2. Someone else is uploading
    • the horizontal bar on the top is moving,
    • I can see the spinner until the actual image comes from the server

The first scenario looks great:

image

But the later looks as follows

kapture 2018-07-09 at 15 39 02

and I find this double indicator an overkill. Too much movement, pure redundancy. Let's stick to the spinner in that situation, WDYT?

cc @dkonopka @oskarwrobel @pjasiun

@dkonopka
Copy link
Contributor

dkonopka commented Jul 9, 2018

☝️ Good catch!

"Infinite progress top bar" should be removed if someone else is uploading image, I just completely forgot to get rid of it.

@pjasiun
Copy link

pjasiun commented Jul 9, 2018

I also did notice it and agree that 2 "spinners" (center and top) is redundancy. But it is not a big issue for me.

@oskarwrobel
Copy link
Contributor Author

oskarwrobel commented Jul 9, 2018

I upload the image

  • the horizontal bar on the top is displaying the progress,
  • I can see the image immediately so there's no need to display the spinner

The spinner is displayed during reading state. When the image is huge then FileReader may need a while to load this image.

@oleq oleq merged commit 997d39b into master Jul 10, 2018
@oleq oleq deleted the t/207 branch July 10, 2018 09:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider changing image upload placeholder
6 participants