[WIP] 564 profile image crop

[ArashDai]

Work in progress, current issue Error: Template parse errors: There is no directive with "exportAs" set to "croppie"

[lmmrssa]

[ArashDai]

todo:
-remove unused croppie and angular-croppie-module packages
-figure out how to pass uploaded image to image source, currently it comes in base64 format

[ArashDai]

current issue:
after selecting a new image and hitting update, you are taken to the user profile page, the old image shows, after hitting refresh the new image appears as it should.
Next:
-Will remove old code after getting some reviews on new code
-make update profile page look better

[Tille88]

This is not obvious to a reader of the code, and should probably not be without commenting. If it is unclear exactly what each step does to you, at least give a reference to where you found the code snippets https://stackoverflow.com/questions/16245767/creating-a-blob-from-a-base64-string-in-javascript

[Tille88]

Took me a little while to figure out what was meant by the lack of update:
For me, the image is changed after clicking 'update', but the small thumbnail in the upper left navbar is not updated.
Maybe the image shown is not updated for others, but that is the behavior I am seeing.

For improvements, I left one comment in the code on improved documentation, and then:

• If the croppie-stuff is not used, it would be better in my opinion to comment it out so I would be certain what code is doing what
• I would still want a 'select crop'-option to give the user the feeling of having it locked down/being in control, before going back to another screen.
• The CSS styling will be important, so moving that up the list of todos for review would make sense IMHO
• For the image update, since it comes from /app/home/home.component.ts => userImageSrc() from database source, so I guess it could be having automatic updates through dataflow if you would alter that to an image component/property in there instead of having an active function call (i.e. userImageSrc)?
  • I'm too new to Angular, but the regular JS-way of doing it to not make things too coupled would probably be some kind of event of image change firing and listened on in a parent-level component through event delegation.
  • Also, I'm too new to give any suggestions on this here, maybe you even deal with events through creating observable 'event streams'? If more than this change would be needed, then instead of having image update event, perhaps having a user detail update event firing on write to db that could be used repeatedly throughout the codebase.
  • The natural Angular way may be to look into how the user._attachment on the userService is included and get rid of the called function through that somehow, since that seems to be the simplest shared state-management tactic...

[Tille88]

Found the issue you described (see bottom of post)

For updated upper left corner update bug:

Minimal footprint would be to emit an event such as the following in user.service.ts:

  set(user: any): any {
    this.user = user;
    if(this.user._attachments **NOT SAME AS BEFORE**){
       this.userImageUpdated.emit();
    }
    this.userChange.next();
  }

But already having userChange.next(), so could just as well piggy back on that, since not very expensive and will not happen all too often. Hence, this seems like a good place to save complexity.

users-update.component.ts calls this in
updateUser(userInfo) { ... this.userService.set(userInfo); ...}
Maybe easiest solution would be to add a variable tracking the state of this and calling this for automatic updates whenever possible.

To not step on your toes, see possible way to change this in this branch building upon yours:
https://github.com/ole-vi/planet/compare/564-ProfileImageCrop...564-small-thumbnail-update?expand=1

Issue described not updated on save
Sometimes it works, sometimes it is not, suggesting some kind of race condition. I did set up a blocking sleep condition before routing back, but it didn't give anything obvious. Still there could be some async action that is not ensured to be completed before routing back, so good luck in your future investigation @ArashDai. However, same part of the codebase I also found this bug that could be of interest: https://github.com/ole-vi/planet/issues/645

[paulbert]

Hi @Tille88 thanks for finding that issue. I'm not sure what is causing the image to fail to update, but it could be related to the issue that the filename is undefined.

So you and @ArashDai know, updating the user profile image in the upper right is done using the Angular suggested method for component interaction via a service (documentation). This utilizes rxjs observables which are like super powered promises if you haven't used them before. In this case we create a stream with the userChange Subject and link it to the userChange$ Observable. We emit events to that stream with this.userChange.next() which will cause any subscriptions to userChange$ to run the function passed to them (see home.component.ts line 52).

You picked out a mistake I made, anyway, that I think we can fix with a separate PR. I didn't think using the function for image src would be an issue, but I was debugging it and realized that it is running with every render. It's probably better to do it like you have and allow Angular to do its own change detection, and combine with this.user = this.userService.get() into one function. See #657

Back to this issue, I think the filename is ending up undefined because we're using the ngx-img package which outputs the file as a blob string without metadata such as the file name. I think this also means we do not need to use prepAttachment from CouchService at all, instead just passing this string in as the 'data' property for the attachment. You will have to also grab the content-type which is in the blob "header" (what you remove in the file.replace) to add that correctly.

[Tille88]

@paulbert yes, it was the undefined image name indeed that was the issue.
Did first remove the prepAttachment functions in couchDB, since I couldn't find it used anywhere else. However it created a merge conflict so... reverted just to be safe.

So going forward, is this roughly what is left to do?:

• Can the prepAttachment funcition safely be removed?
• Remove everything not needed (croppie-stuff and double check all functions used somewhere, and similar clean up)
• 'Select crop option' happens automatically on altering that, maybe not needed to alter.
• Check how works on other devices?
• Improve styling perhaps for different screen sizes?

[Tille88]

Also, some things that needs more permanent solution in there, but it is working again [way to generate a key, increment previous or generate ID, if id generation, where to put it in the codebase].

[paulbert]

@Tille88 Thanks for working on this. To answer your questions:

• No, that is used by other parts of the application (with input type="file" elements)
• Cleanup sounds good, but be careful since the dependecies for ngx-img don't seem to install properly so we should include them in our package.js
• I'm not sure what you mean by 'select crop option'. Do you mean the image starts cropped by default? It would be nice to have the crop start with

As for the last two we should check different devices and make issues to improve the styling. This is already a big change, and it'll be nice to have this functionality on the master branch.

[Tille88]

@paulbert Thank you for the reply
Your sentence there got cut off, crop start with ...?

• That means, I should be able to remove all references to 'croppie' in package.json? But not the two dependencies, correct? (npm lists 0 dependencies, did have to look at the package github to see what you meant)
• Remove some imports from angular core (AfterViewInit, ViewChild) that ended up being unused.
• Need to solve the issue with having this included though: Maybe best would be to start as 'img' and then just increment to 'img1' img2' etc on each change if there are no helper function modules or actual id generation mechanism to use:

uniqueId(): string {
    return 'id-' + Math.random().toString(36).substr(2, 16);
  }
[...]
attachments[this.uniqueId()] = [...]

• Lastly, this is something I'm not sure will hold in use, but that is what testing is for eventually :)

// Unclear if only encoding is base64
// This ought to cover any encoding as long as the formatting is: ";[encoding],"
const imgDataArr: string[] = this.file.split(/;\w+,/);

Will test out the first two bulletpoints during tonights meeting and also if there is a default crop level that would be preferred. For the 3rd and 4th, I would like some input before making alterations.

[Tille88]

We could fork and change the initial crop of the image if that is high enough on the priority list.
It doesn't look too hairy (see initializeCrop-function), but also maybe not super fun to deal with a private fork throughout... since a lot of options are exposed, but this particular one seems maybe like it doesn't.

Options
https://harryy2510.github.io/ngx-img/home

Component controller sourcecode
https://github.com/harryy2510/ngx-img/blob/master/src/module/component/ngx-img-crop.component.ts

[paulbert]

I added a small change since handleAttachment isn't very useful anymore (before needed to wait for async file load, now that is taken care of in the ngx-img package).

Once this passes checks I'll merge it.