-
Notifications
You must be signed in to change notification settings - Fork 8.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
[FileUpload] Enhance file-upload directive #6842
Conversation
link: function ($scope, $elem, attrs) { | ||
var onUpload = $parse(attrs.fileUpload); | ||
var $button = $elem.find('.upload'); |
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.
Hmm, seems to me like this is a slightly odd API... So essentially if I include something in the directive that has a class of "upload" it will be turned into something clickable with a file input. I'm not sure what the best alternative is, though, because maybe the consumer of this directive would want a link, or a button, so you can't just look for all links or all buttons either... I just feel like this should be part of the API somehow (like one of the scope
values, or something). I don't know for sure, I just think this seems a little odd. I'll think on it some more.
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.
I guess you're right, it is a bit odd... my goal was to give the transcluded content full control over how the clickable element is displayed. Not sure how else to do it at the moment but I'll give it some thought as well.
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.
I thought about this more, and I thought, what if there was something like
scope: {
onRead: '&',
onLocate: '&',
label: '=',
uploadSelector: '@'
}
and then
var $button = $elem.find($scope.uploadSelector);
Thoughts? I would just prefer it be a more explicit part of the API of the directive.
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.
Seems like a great idea, I'll make the update.
Adding features for Add Data CSV usecase; * Drag and drop support * Gives parent access to File object * Moved from attribute to element type directive to allow custom content * Hide the file input better
c8c0694
to
6fce94e
Compare
@lukasolson finally got around to making that update, please take another look. |
LGTM |
Supports #6845. You can actually see these updates in action on the first step of the CSV upload wizard in that pull.
Adding features for Add Data CSV use case;
Assigned to @lukasolson since it looks like he was the original author of this directive.