Feature/536 update edit page

elislaats commented 2 years ago

Description

Updated the Edit page according to issue #536 , while also fixing #561

Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[x] This change requires a documentation update

Checklist

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[x] I have added tests that prove my fix is effective or that my feature works
[x] I updated the changelog with an end-user readable description
[x] I assigned this pull request to the correct project board to update the sprint board

Steps to Test or Reproduce

Go to the project overview page and click on some filters
Click on a project that you can edit
Click on the edit button
See the new and improved edit page 😎
Go trough all the steps to see everything works.
Save your changes to see the detail page (& overview in the background) get updated after saving 🎈
Click on some filters to see that they don't get removed after editing. 🦸 Bonuspoints: Check same thing in different browsers and mobile 😄

Link to issue

Closes: #561 Closes: #536

waltersajtos commented 2 years ago

I'm not entirely sure if I understand what you mean by updating the overview page in the background?

Do you want to update the project info of the updated project? If that's the case, you could subscribe to the projectUpdatedevent in the overview page the same way we do for the likes.

Let me know if I misunderstood you.

elislaats commented 2 years ago

@waltersajtos rfirst of all, thank you very much for taking the time to review my code. I already fixed a couple of the things you pointed out.

I'm not entirely sure if I understand what you mean by updating the overview page in the background?

Do you want to update the project info of the updated project? If that's the case, you could subscribe to the projectUpdatedevent in the overview page the same way we do for the likes.

Let me know if I misunderstood you.

I think you're understanding me well. I also understand what you mean, but I'm just struggling to find which event to subscribe to exactly.I don't see anything usable named projectUpdated.

I looked into how it's done with the likes, but I'm having some trouble re-creating that for the whole project-card. If you have any insights that might help me about what variables/subjects to use that would be great.

Thanks again!

waltersajtos commented 2 years ago

You could either take the updatedProject (details.component.html:57), which from my understanding means that the update/ save button was clicked on the edit page. And pass that project to the overview page with another @Output in the details component. Then on the overview page, check if it's on the page and if it is, update it.

If you want to get fancy you could also implement the @Output in the project-service instead of in the views. This allows you to subscribe to the same event from both the overview page and the detail page.

elislaats commented 2 years ago

@waltersajtos I'm very grateful for your help again. I'm just still not really understanding it well enough.

You could either take the updatedProject (details.component.html:57), which from my understanding means that the update/ save button was clicked on the edit page. And pass that project to the overview page with another @Output in the details component. Then on the overview page, check if it's on the page and if it is, update it.

Please tell me if I'm wrong, but I don't think this is possible. I don't think the details component is a child of the overview page, which is needed to use the @Output functionality

If you want to get fancy you could also implement the @Output in the project-service instead of in the views. This allows you to subscribe to the same event from both the overview page and the detail page.

I think this is the way to go, but I'm not understanding how to use the services well enough. I'm trying to find resources about it but I'm not really finding anything helpful. Could you help explain it to me? Maybe by sending me resources about it or we could plan a call if you have some time for that 😄

Otherwise maybe @MeesvanStraten or @DaveBouman can look at it and/or help me (or know someone currently in the team who's able to?)

Thanks again!

waltersajtos commented 2 years ago

@juliaslaats yeah ur right, I'll look for some resources or explain it if i can't find any. Call is going to be hard, I'm at my internship from 9.30 till 18.

waltersajtos commented 2 years ago

A service is nothing more than a bridge between the backend and the front-end. Generally speaking, it contains most of the business logic that the application needs. Multiple components can use a service at the same time.

The project service for example is used by the detail page, overview page, and the user-project page. They all need the same logic, by using the service we can re-use the code very easily.

Another useful thing about services is that, you can share a state between components since lives outside of the components. In the wizard, for example, every wizard page updates the project with the properties that were set on that page.

In order to add an output to the project-service you can add the following:

    @Output() projectUpdated$: EventEmitter<boolean> = new EventEmitter();

    put(id: number, project: Project): Observable<Project> {
        this.change.emit(project)
        return super.put(id, project)
  }

All this does is emit the event and then call the put function in the class it inherited the function from. The Observable's can be a bit confusing sometimes but you shouldn't have to worry about it for now.

Then you can use it in the component as follows:

    this.projectService.projectUpdated$.subscribe(project => {
        // Update the component here
    })

This subscription is fired whenever the Output is emitted. It's some under the hood angular magic again.

You can either implement this in the ngOnInit or subscribe when a detail page is opened and unsubscribe when the detail page is closed. If I were you I would put it in ngOnInit since it won't have a big impact on the performance anyway. I typed this in a few minutes, it might be a bit on the tangent. I hope it helped clearing things up! Let me know if anything is unclear.

elislaats commented 2 years ago

@waltersajtos @MeesvanStraten I think I fixed it!

I used the @Output() in the project-service, and subscribed to that in the overview. I didn't use the .put() because that seemed to be protected or something, but I found a way around it.

Not sure if it's the prettiest solution but would love to hear what you think.

I'm now I'm working on the issue with duplicate styling and when that's finished I think I'm ready for a real PR 🙏

MeesvanStraten commented 2 years ago

@juliaslaats yay, I will check it out today or latest tomorrow 😃

waltersajtos commented 2 years ago

@waltersajtos @MeesvanStraten I think I fixed it!

I used the @Output() in the project-service, and subscribed to that in the overview. I didn't use the .put() because that seemed to be protected or something, but I found a way around it.

Not sure if it's the prettiest solution but would love to hear what you think.

I'm now I'm working on the issue with duplicate styling and when that's finished I think I'm ready for a real PR 🙏

It's not a huge issue if it isn't done in the service but it would be a lot cleaner if it is emitted by the service. Can you show the code that you tried? As I mentioned before the observables that the put function expects and returns are a bit odd sometimes. I might be able to help you out.

If it's a lot of work then i think this approach is fine. Looks very nice tho, great work!

elislaats commented 2 years ago

@waltersajtos

It's not a huge issue if it isn't done in the service but it would be a lot cleaner if it is emitted by the service. Can you show the code that you tried? As I mentioned before the observables that the put function expects and returns are a bit odd sometimes. I might be able to help you out.

I tried using the code you sent, but I got all of these errors that I don't fully understand. This is why I reverted to doing it the way I did it now.

If it's a lot of work then i think this approach is fine. Looks very nice tho, great work!

I think it might be nice to just leave it like this for now. It works well.

waltersajtos commented 2 years ago

If @MeesvanStraten and @DaveBouman agree with leaving it like this it's alright I guess. It's just a little bit confusing that the component is calling the event while the service can do it too. Not a huge deal but should be considered changing.

elislaats commented 2 years ago

If @MeesvanStraten and @DaveBouman agree with leaving it like this it's alright I guess. It's just a little bit confusing that the component is calling the event while the service can do it too. Not a huge deal but should be considered changing.

@waltersajtos I understand that it would be neater the way you suggetsted. But I'm just really not sure how to make that happen 😞 I also don't think anyone working on DeX right now is able to help me with it. So then I feel like if it works, it works and it might just be good enough. I feel also like I've stared at this enough now, but if Mees and Dave disagree with me I'll get over it and try to figure out the observable to create a more neat way...

DaveBouman commented 2 years ago

I will look into it today if the pull request is OK. Or that the changes Walter Wants are needed.

DaveBouman commented 2 years ago

I do think that Walter is right about that it should be in the service. For the moment I will accept it so that it can be merged on time for the next sprint delivery. Perhaps @juliaslaats we could sit together this or next week to better figure out how to refactor it to the service. It is a good solution but we can make it better! :)

DigitalExcellence / dex-frontend