Basic vue UI #133
Basic vue UI #133
Conversation
added some more structure to the files
renamed views to avoid conflicts with api objects
ui/src/ApiThing.ts
Outdated
| email: string; | ||
| } | ||
|
|
||
| class ApiThing { |
There was a problem hiding this comment.
You can create singletons in js by
const apiThing = new ApiThing();
export apiThing;This lets you just import {apiThing} from './.../ApiThing' and you don't have to pass it through all components!
There was a problem hiding this comment.
ApiThing is not a good name, and does too many things at once. I'd split up in AuthApi, PhotoApi and UserApi
There was a problem hiding this comment.
I agree on the name, I already renamed it to WahlzeitApi, just didn't commit that yet.
I made it a single class, since I'd say it only does a single thing, that being handling the interaction with the backend API (one class for one API).
Splitting that would require some base class that handles the basic communication that would be shared by all the individual API's.
I think that any code using a singleton is bad code for multiple reasons, but if it is the preferred solution in your project I can change that.
If I do split the the API class, that would make 3+ singletons.
There was a problem hiding this comment.
Well, I agree that the singleton approach is not optimal as well. In general, dependency injection is the way to go in my opinion. Using the props is a way to inject dependencies... However, I'd still go with the instance export way because it makes the component interface smaller. But maybe I'm wrong - did you look into best practices for vue regarding this?
Single-Responsibility is always a matter of perspective... Since the API is still slim, lets leave it as you designed it.
There was a problem hiding this comment.
Vue has provide/inject for dependency injections, but that has some limitations and since I use the api in almost every component anyways I chose props. The documentation also isn't very clear on how to use it with Vue 3 and TS class components, and due to time constraints I couldn't do a lot of testing.
There are some plugins that would help here, but I didn't want to add those as additional dependencies.
I might just change it to a singleton for reading simplicity.
For the Single-Responsibility I just think that adding multiple classes with only 2-3 methods each would be overkill.
ui/src/views/PhotoView.vue
Outdated
|
|
||
| mounted() { | ||
| this.api | ||
| ?.getPhoto(this.id) |
There was a problem hiding this comment.
Pls use the aync/await syntax for better code readablity: typescriptlang.org/docs/handbook/release-notes/typescript-1-7.html
There was a problem hiding this comment.
Same for other views
There was a problem hiding this comment.
Not sure I understand what you mean.
So replace the use of promises with await?
The link you posted just leads me to typescript.org
There was a problem hiding this comment.
Yes, those promises look like spaghetti code, so please use the async/await syntax instead.
renamed class using singleton to reduce used props using await instead of promises
Added signup modal Added search functionality (currently broken) cleaned up the codebase
|
Hey Hendik, I just checked out your PR and saw a few issues in my editor: (1) Please rebase your PR on the |
This is based on the new backend by @knusperkrone