-
Notifications
You must be signed in to change notification settings - Fork 4
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
Connect profile API #10
Conversation
Could you please attach a few screenshots? Would be easier to discuss general directions if I could glimps without having to build and deploy everything. |
I've been thinking about this PR for a bit, but all in all I'm a bit wary of the general methodology. The original UI I made was a PoC to have an idea of what's needed, but since then those were evolved a bit in #1. Would be nice to take those discussions into consideration too, otherwise we'll have two diverging idea sets. Re this PR a few issues I mentioned before in #11 (comment):
A general issue that I'm afraid of with this PR is that it kind of hacks in network requests, handling and whatnot all together. Yes, it's good for a fast PoC, but I won't be able to maintain such a code base. I kind of feel that the approach @daodesigner is taking in #12 is a bit more elegant, as the network requests are completely separate from rendering. I'm also more of a building from the ground up person. The correct approach I think would be to define the screens that are needed and then go about implementing them. Since @daodesigner started defining them, it might make sense to collaborate (or build the bases for that) than to try and make the current UI work, which will be dropped either way. |
I just checked PR #12. I completely agree with your decision. I would like @daodesigner to continue the design implementations. I will keep an eye on further development there. Let's continue the discussion on PR #12 and let me see what I can do there. 👍 |
PUT
user info into profile