-
-
Notifications
You must be signed in to change notification settings - Fork 10
Adds Team Member View #64
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
Conversation
adamoxley
left a comment
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.
Haven't had a chance to actually run the code yet, but a general look through the code looks good. Nicely done! 👍
Left a few small comments but nothing that would stop this from being merged, would be good to have some others take a look at this change.
| guard let path = Bundle.main.path(forResource: "about", ofType: "json"), | ||
| let data = NSData(contentsOfFile: path) as Data? else { | ||
| errorMessage = "Could not find about.json file" | ||
| isLoading = false | ||
| return | ||
| } |
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.
This would be a good one to move to our backend, but being this close to the date it probably wouldn't get done 😄 I guess the team is easy enough to update if it ever changes anyways and having the structure already defined on front end is good to have if we did move it to the backend.
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.
This is my main concern with a few of these PRs, we really need to be API driven and not locking ourselves into slower releases and opening up to inconsistency between mobile and web.
Less critical for a staff page but as a key principle
SwiftLeeds/Views/About/about.json
Outdated
| { | ||
| "name": "Adam Oxley", | ||
| "role": null, | ||
| "linkedInURL": "https://www.linkedin.com/in/adam-oxley-41183a82/", | ||
| "twitterURL": "https://twitter.com/admoxly", | ||
| "slackURL": "https://swiftleedsworkspace.slack.com/team/U02DRL7KUCS", | ||
| "photoURL": "https://swiftleeds.co.uk/img/team/oxley.jpg" | ||
| }, | ||
| { | ||
| "name": "Adam Rush", | ||
| "role": "Founder and Host", | ||
| "linkedInURL": "https://www.linkedin.com/in/swiftlyrush/", | ||
| "twitterURL": "https://twitter.com/Adam9Rush", | ||
| "slackURL": "https://swiftleedsworkspace.slack.com/archives/D02ELG76VC0", | ||
| "photoURL": "https://swiftleeds.co.uk/img/team/rush.jpg" | ||
| }, |
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 personally feel that @adamrushy should be first in the list 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.
We randomise order on the website for exactly this.. no one has priority/preference. All equals.
| #if os(iOS) | ||
| let columnCount = UIDevice.current.userInterfaceIdiom == .pad ? 3 : 2 | ||
| #else | ||
| let columnCount = 2 | ||
| #endif |
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.
Could this not be simplified without the if os(iOS) check since it defaults to 2 if it's not .pad anyhow? 🤔
Dependent PRs
SwiftLeeds/swiftleeds-web#159