Skip to content
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

Include multiple abstractions to enable custom implementations #16

Open
shaps80 opened this issue Feb 21, 2023 · 8 comments
Open

Include multiple abstractions to enable custom implementations #16

shaps80 opened this issue Feb 21, 2023 · 8 comments

Comments

@shaps80
Copy link

shaps80 commented Feb 21, 2023

Hey mate, I love your work and I think is a great idea. But I'm curious if there's any reason you wouldn't have implemented this library as an easy way to add voting to the app (will show an example below) without enforcing specific UI?

// the AppStore rating implementation is a nice API IMO
@Environment(\.votingRequest) private var vote
// ...
try await vote("someId")

The UI elements could be an optional added-benefit for those that want to use them wholesale but I feel like the basic feature of just "voting" adds a lot of value as-is. In my experience the styling approach you've chosen doesn't scale well nor to everyone's needs, I think you can see this already in the Issues actually.

So if you were going to provide it as a convenience perhaps you could opt for a couple lower level APIs that you simply build on top of, but make it easy for consumers to build fully custom implementations of their own as well.

// Borrowing from `FetchRequest` CoreData property wrappers, namespace could also be provided, or defaulted perhaps to the Bundle ID
@FetchVotes("someId") private var votes

Then you could use the above to build UI on top, at which point I'd then provide a more SwiftUI-y Style as-in how ButtonStyle, etc are implemented.

Anyway, no criticism just genuinely inspired me a little here and I thought I'd share how I would have approached the problem, perhaps its helpful or inspires some new work 👍

@jordibruin
Copy link
Collaborator

Good feedback. We wanted to create the simplest implementation that someone could add in 2 minutes with a very clear purpose and usecase.

If we would make it more low level there would not be a very straight forward path for someone to add this without a lot of their own work correct?

Can you explain what options don't scale with the current method?

@shaps80
Copy link
Author

shaps80 commented Feb 21, 2023

You could keep the current implementation as a higher level abstraction – cause I get the use case and I agree its a great idea 👏

As to my other comments, I just mean if you check your Issues you can see already that most if not all request are for more customisation, that's difficult to keep adding to, since it quickly goes beyond any reasonable scope without being extremely project specific.

Screenshot 2023-02-21 at 12 24 55

@shaps80
Copy link
Author

shaps80 commented Feb 21, 2023

Again, I love the idea and simplicity of this, I was just suggesting that exposing relevant API that's not bound to the UI, enables more custom implementations to exist while still benefiting from your original conception, which is great.

But again, you can have the best of both worlds 👍 and better yet, your UI bits would benefit from the same APIs.

@jordibruin
Copy link
Collaborator

@shaps80 I made (or brought them up in our chat) most of the issues that you highlighted, and we plan to add all of them within the current implementation.

I think we don't want to create a solution that handles each edge case, so we'll definitely have to make choices here and there to not overcomplicate things.

That being said, we made this over the last 2.5 days, so we also just wanted to keep it simple and launch so we could get feedback. Would be curious to hear what features you would want to add that aren't possible with the default approach.

@shaps80
Copy link
Author

shaps80 commented Feb 21, 2023

To be honest if I wanted to implement something like this in my apps (especially for a client), I'd need the design to 'fit' – basic styling changes generally won't be enough I'd argue in most cases, but lets say at least some.

Its totally fine btw if you want to disregard my ideas I'm not attached to them lol.

Its a great idea and you've executed nicely overall 👍 Just thought I'd add to the mix in case its helpful, keep up the great work, sorry for complicating it haha 👍

If you're open to PRs and I have some spare time I'd be happy to make a PR that suggests the changes – maybe that clarifies better, and if not you can always reject/close it 👍

Also, feel free to close this comment – I don't want to clutter up your Issues

@jordibruin
Copy link
Collaborator

Don't get me wrong, I appreciate the feedback! Just trying to explain the rationale behind some choices, but maybe I came across defensive, sorry!

Would love to see a PR that tackles this. Maybe you can rename this issue to something like "Make more flexible" so that we can use this issue to discuss what kind of things would be useful to expose!

@shaps80
Copy link
Author

shaps80 commented Feb 21, 2023

No I didn't get that feeling at all, all good 👍 – Textual conversation is much hard right? Haha.

I'll rename for you, thanks.

@shaps80 shaps80 changed the title Questions Include multiple abstractions to enable custom implementations Feb 21, 2023
@jordibruin
Copy link
Collaborator

@shaps80 have a look at this 👀

Would love your feedback!

#38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants