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
The beginnings of a public package page. #65
Conversation
… for the home page.
…iftPackageIndex-Server into public-package-page
# Conflicts: # Sources/App/routes.swift
This page almost certainly won’t exist in the final version, it’s just for testing.
|
||
func index(req: Request) throws -> EventLoopFuture<Response> { | ||
// No such thing as a full index of packages. | ||
return req.eventLoop.future(req.redirect(to: "/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, I had no idea there's an eventLoop.future
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, when you've been doing Vapor development as long as I have, you learn a few things... 😂 Orrrrr... Google is awesome 🤷♂️
Actually I learned something cool about redirects when figuring this out. My initial feeling was just to make this a 302, but a 303 enforces the redirect to be a GET regardless of the method used for the initial request. It's not actually changing anything here as index
is always a GET in this case, but it's good to know.
let package: Package | ||
|
||
init(_ package: Package) { | ||
self.package = package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess here's where we interface with our view model. After tackling #62 - depending on how that goes 😅 - shall I start filling in the query and plug it in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that'd be wonderful. 👍
My gut feeling says that the view model (for example) shouldn't construct the full sentences that are going to be used, but instead populate everything that might be needed.
Maybe something like this:
To construct this sentence:
"In development for over 5 years, with 1,433 commits and 79 releases."
The view model could contain:
- First commit date (
Date
) - Number of commits (
Int
) - Number of releases (
Int
)
and to construct this sentence:
"By Christian Noon, Mattt, Jon Shier, Kevin Harwood, and 186 other contributors."
- Array of:
- Full Name (
String
) - Profile URL (
URL
)
- Full Name (
- Number of other contributors (
Int
)
Is that what you were thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the place for this discussion isn't a merged PR. I'm going to make a new issue for this so we can discuss it and track it.
This looks great, can't wait to hook this up with data and start making the content browsable! |
I'm submitting this for merging now as I'm not going to be able to work on this tomorrow (newsletter!) and it's already getting big so I think it's best to get it in to avoid messy merges.
The basic structure of the design is done, but much of the detail (for example all the icons) are not yet complete. It's coming along really well though.
It's far from finished, and the most obvious thing that's missing is a structure to the code in
PackageShowView
. I know how I want to split that up, it's just not done yet and I'm finished for today.Code review is very welcome with the exception of what I mentioned above.
In terms of how it looks... Here it is:
and for mobile: