BUG: File Uploading Notifications (fixes #7883) #880

Merged
merged 1 commit into from Nov 2, 2012

Conversation

Projects
None yet
2 participants
Contributor

adrexia commented Oct 16, 2012

  • Put "File upload complete" and "back to folder" together. Turned 'File upload' into a message, and updated the message styles.
  • Moved allowed file types into the area where users are uploading files. This is a temporary fix until js tooltips are implemented, at which point, these details will be shown when clicking a question mark beside "Choose files".
  • Added transition and animation mixins to framework.
  • Added small animation effect to files when opening iframe to edit. Now slides down, rather than just appearing open

Linked to silverstripe/silverstripe-cms#223

adrexia referenced this pull request in silverstripe/silverstripe-cms Oct 16, 2012

Merged

BUG: File Uploading Notifications (fixes #7883) #223

@chillu chillu commented on an outdated diff Oct 16, 2012

templates/AssetUploadField.ss
@@ -1,9 +1,15 @@
<div class="ss-uploadfield-item ss-uploadfield-addfile field">
- <h3>
+ <h3 class="has-tool-tip">
@chillu

chillu Oct 16, 2012

Owner

Inconsistent spelling, class is called "tooltip" without a dash elsewhere

Owner

chillu commented Oct 16, 2012

Looks much nicer this way, thanks Naomi! I'm happy to merge the changes, apart from the tooltip. I don't see the value in making this a CSS3 solution, rather than using a JS library, for a number of reasons:

  • It forces us to use non-semantic elements for tooltips (rather than the "title" attribute)
  • It doesn't work well with touch devices (not click->close pattern)
  • Its not accessible (visibility: hidden, no aria-describedby)
  • It doesn't provide much advantage in terms of animation performance, since its a simple fade-in
  • Harder to control and customize via CSS (rather than JS + HTML5 data attrs)
  • Its uglier to implement in terms of what the dev comes in contact with, e.g. the z-index magic which should be handled centrally by the UI.

So, given that we'll want to use tooltips consistently throughout the whole UI (for headlines, form elements, etc),
I'd suggest we remove this from the pull request, and handle it separately through http://open.silverstripe.org/ticket/6898. My current thinking is to use jQuery UI tooltips: http://jqueryui.com/tooltip (which requires us to upgrade to jQuery UI 1.9 first).

Contributor

adrexia commented Oct 16, 2012

Sounds good to me. I wasn't really sure where I'd put any javascript for the tooltip, so stuck to css.

Re: upgrading to the latest jQuery, what sort of things should be tested when we do that? Are you planning on doing that yourself, or could I do it?

Owner

chillu commented Oct 16, 2012

I've started the upgrade (to an earlier beta release) here: https://github.com/chillu/sapphire/tree/jquery-1.9 and https://github.com/chillu/silverstripe-cms/tree/jqueryui-1.9

It'll probably require an upgrade to jQuery 1.8 as well, so we'll need to pretty much retest the whole UI. Tabs will be one main focus, and running the entwine tests against 1.8.

Contributor

adrexia commented Oct 16, 2012

Updated to remove tooltips.

I repositioned the file details to below the upload area for now, as it'll be easier to move it from there once tooltips have been implemented. I also included a few lines of styling to stop it looking too ugly. That can be removed once we have the ability to use js tooltips.

Contributor

adrexia commented Oct 31, 2012

Is this ok to pull now?

@chillu chillu and 1 other commented on an outdated diff Oct 31, 2012

admin/scss/_mixins.scss
@@ -101,5 +101,92 @@
*:first-child &{ zoom:1;}
}
+//**----------------------------------------------------
@chillu

chillu Oct 31, 2012

Owner

Are all those mixins still required now that we've removed the tooltip code?
They look fairly general purpose, but also need documentation to explain
what they're actually doing - its a bit much complexity to add "just in case".
Does anything speak against removing them?

@adrexia

adrexia Oct 31, 2012

Contributor

Good point, we probably don't need them. They are general purpose, but if we have no need for css animation, then we don't need them here. More complex animations we'd probably have to use javascript for anyway, as a lot of the actions are already javascript*.

I'd be tempted to leave the duration mixin in there, and remove the more complex animation keyframe stuff (and delay, as the use case isn't as general purpose). The duration mixin lets you do simple ui stuff like transition hover changes easily. It might be worth using this effect on our button hover? It might not be worth it though. I can remove them all for now if that's easier?

(* I think it would be nice to animate the expanding and shrinking of panels, particularly the preview as the change is a little jarring.)

@chillu

chillu Oct 31, 2012

Owner

I'd say remove it all until we have an implemented use case somewhere.
Good point about the performance on panel animation,
I think Hamish has some JS-related optimizations there,
so we'll see how jarring they still are first.

On 1/11/2012, at 12:00 AM, Naomi Guyer notifications@github.com wrote:

In admin/scss/_mixins.scss:

@@ -101,5 +101,92 @@
*:first-child &{ zoom:1;}
}

+//*----------------------------------------------------
Good point, we probably don't need them. They are general purpose, but if we have no need for css animation, then we don't need them here. More complex animations we'd probably have to use javascript for anyway, as a lot of the actions are already javascript
.

I'd be tempted to leave the duration mixin in there, and remove the more complex animation keyframe stuff (and delay, as the use case isn't as general purpose). The duration mixin lets you do simple ui stuff like transition hover changes easily. It might be worth using this effect on our button hover? It might not be worth it though. I can remove them all for now if that's easier?

(* I think it would be nice to animate the expanding and shrinking of panels, particularly the preview as the change is a little jarring.)


Reply to this email directly or view it on GitHub.

@adrexia adrexia BUG: File Uploading Notifications (fixes #7883)
Put "File upload complete" and "back to folder" together. Turned 'File
upload' into a message, and updated the message styles.
Moved allowed file types into the area where users are uploading files.
This is a temporary fix until js tooltips are implemented, at which
point, these details will be shown when clicking a question mark beside
"Choose files".
Added small animation effect to files when opening iframe to edit. Now
slides down, rather than just appearing open
Linked to silverstripe/silverstripe-cms#223
2dabaeb
Contributor

adrexia commented Nov 1, 2012

I have removed the animation/effects mixins. :)

Owner

chillu commented Nov 2, 2012

Tested in IE7 and Chrome, merged!

@chillu chillu added a commit that referenced this pull request Nov 2, 2012

@chillu chillu Merge pull request #880 from adrexia/fileoverview
BUG: File Uploading Notifications (fixes #7883)
70352fb

@chillu chillu merged commit 70352fb into silverstripe:master Nov 2, 2012

1 check passed

default The Travis build passed
Details

@stojg stojg pushed a commit to stojg/sapphire that referenced this pull request Jul 24, 2014

@mateusz mateusz Merge pull request #880 from halkyon/fulltextsearch_text
ContentControllerSearchExtensionTest doesn't clean up after itself.
27c8122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment