-
Notifications
You must be signed in to change notification settings - Fork 20
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
Go File Upload #149
Go File Upload #149
Conversation
1292967
to
3b1e130
Compare
37263a3
to
ac6bebd
Compare
@StevenUlmer we've already discussed this a bit, but can you look into what it would take for us to make the file upload process something like that InVision mockup? |
b26983b
to
cd2e8fd
Compare
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.
projects/go-lib/src/lib/components/go-file-upload/go-dragon-drop.directive.ts
Outdated
Show resolved
Hide resolved
projects/go-lib/src/lib/components/go-file-upload/go-dragon-drop.directive.ts
Outdated
Show resolved
Hide resolved
4ae5c39
to
4cf02f9
Compare
@grahamhency I think this is ready for another look |
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.
Got a few items to attend to, let me know if you have questions. This could also do with a rebase.
export class DragonDropDirective { | ||
@Output() fileDropped: EventEmitter<any> = new EventEmitter<any>(); | ||
|
||
@HostBinding('class.go-file-upload--active') active: boolean = false; |
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.
@StevenUlmer what if we extracted this out into it's own thing? We could pass in the go-file-upload--active
class as an @Input
here and then handle the drop
event in the file-upload
component as an event handler.
We don't have to in this PR, could always extract it out later. But that would make this drag/drop functionality reusable which would be nice.
Thoughts?
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 went ahead and pulled out al the functionality I could. I had to do some stuff around using class here because just using @HostBinding('class')
overwrites all classes already applied to the element
<span class="go-file-upload__container--loading-text">Uploading File...</span> | ||
</div> | ||
<div class="go-file-upload__container go-file-upload__container--selected" *ngIf="state === 'selected' && !isLoading"> | ||
<go-icon icon="check" iconModifier="positive" iconClass="go-icon--large"></go-icon> |
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.
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 don't think my PR effects this. However, if I'm not mistaken, while the go-button
takes "positive" as an option for its buttonVariant
input (and converts it internally to "primary"), the go-icon
only accepts "primary" for its iconModifier
input, since it has no internal logic which converts "positive" to "primary" and only has a go-icon--primary
class, and not a go-icon--positive
class.
So I don't think iconModifier="positive"
will do anything. You probably want iconModifier="primary"
, if anything, although all that does it set the color
to black, which is probably unnecessary due to inheritance.
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.
@jaredami you make a good point here. I think it might make more sense to set the color via a class from the go-file-upload
component itself. Thought's @StevenUlmer ?
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.
Ya that makes sense. I added a class in the go-file-upload
component to set the color
projects/go-lib/src/lib/components/go-file-upload/go-file-upload.component.scss
Outdated
Show resolved
Hide resolved
projects/go-lib/src/lib/components/go-file-upload/go-file-upload.component.html
Outdated
Show resolved
Hide resolved
projects/go-lib/src/lib/components/go-file-upload/go-file-upload.component.scss
Outdated
Show resolved
Hide resolved
projects/go-lib/src/lib/components/go-file-upload/go-file-upload.component.scss
Outdated
Show resolved
Hide resolved
@Input() label: string; | ||
@Input() multiple: boolean = false; | ||
@Input() state: 'selecting' | 'selected' = 'selecting'; | ||
@Input() theme: 'light' | 'dark' = 'light'; |
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.
This binding exists, but it doesn't seem this actually works as intended. Can you take a look at the styling for a dark
theme and update them to be correct please?
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.
We'll also want documentation for this.
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 fixed the dark theming here and added documentation for it.
projects/go-tester/src/app/components/test-page-3/test-page-3.component.html
Outdated
Show resolved
Hide resolved
4cf02f9
to
794ccc3
Compare
*Updates styles of the file upload *Adds ability to differenciate between single and multiple file uploads
26e9e12
to
63c737d
Compare
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.
👍 This looks good to me! We'll see how this goes in practice 😂
I am confident it will work well though 🏆
This attempts to close #127