Skip to content
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

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

Merged
merged 1 commit into from Nov 2, 2012

Conversation

adrexia
Copy link
Contributor

@adrexia 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

@@ -1,9 +1,15 @@
<div class="ss-uploadfield-item ss-uploadfield-addfile field">

<h3>
<h3 class="has-tool-tip">
Copy link
Member

Choose a reason for hiding this comment

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

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

@chillu
Copy link
Member

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).

@adrexia
Copy link
Contributor Author

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?

@chillu
Copy link
Member

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.

@adrexia
Copy link
Contributor Author

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.

@adrexia
Copy link
Contributor Author

adrexia commented Oct 31, 2012

Is this ok to pull now?

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

//**----------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

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.

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
@adrexia
Copy link
Contributor Author

adrexia commented Nov 1, 2012

I have removed the animation/effects mixins. :)

@chillu
Copy link
Member

chillu commented Nov 2, 2012

Tested in IE7 and Chrome, merged!

chillu added a commit that referenced this pull request Nov 2, 2012
BUG: File Uploading Notifications (fixes #7883)
@chillu chillu merged commit 70352fb into silverstripe:master Nov 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants