forem / DEV-ios

DEV Community iOS App
GNU General Public License v3.0
357 stars 97 forks source link

General Cleanup: details below #211

Closed karnett closed 4 years ago

karnett commented 4 years ago

What type of PR is this? (check all applicable)

Description

Screen Shot 2020-03-03 at 10 31 12 AM

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Simulator Screen Shot - iPhone Xʀ - 2020-03-03 at 10 50 10

Added to documentation?

[optional] What gif best describes this PR or how it makes you feel?

snow white cleaning

karnett commented 4 years ago

That last SwiftLint will require a bit bigger refactor.. I'll put up a different PR to fix SwiftLint first.

karnett commented 4 years ago

https://github.com/thepracticaldev/DEV-ios/pull/212

fdocr commented 4 years ago

Hi @karnett thank you for sending in your PR! I see the value of the directory restructure, specially since we have things like our ViewControllers currently living in a folder named views.

I think this is a change that requires some general discussion as in the end there is no absolute right or wrong here. For starters (and these are just my ways of thinking):

  1. How would you feel about taking AppDelegate.swift out from the Utilities folder into the root Directory? I've always had the sense that since it's the entrypoint of the app it's generally not just a 'Utility' class
  2. This is a very lightweight project (total number of ViewControllers, Storyboards, Assets, etc). This is why I feel nesting assets & storyboards into Resources is a bit of an overkill. Maybe keeping them as folders in the root directory might even feel cleaner and slightly more "reachable"?
  3. I really like the changes to the constraints in the LaunchScreen to center the logo. I would even encourage you to open a separate PR for this to get it merged sooner :)
karnett commented 4 years ago

Hey @fdoxyz

  1. Definitely agree its not just a utility class, but I've always found its best to not have it front and center - people generally like to put things in there that don't belong.
  2. My thought here was being a web app, we're not really changing assets or storyboards too frequently.

Agree there's no right or wrong here - but this felt a lot cleaner to me. If you feel strongly on either 1 or 2, happy to make changes. Still hesitant about 1 :)

fdocr commented 4 years ago

I definitely find your arguments as valid @karnett. There's one thing about 1 though:

Definitely agree its not just a utility class, but I've always found its best to not have it front and center - people generally like to put things in there that don't belong.

Absolutely agree that adding unnecessary code to AppDelegate is an anti-pattern. The fact that it's front and center shouldn't affect that though, in the sense that PRs should be reviewed before a merge. Unnecessary clutter should not be allowed and alternatives suggested on code reviews.

Despite this discussion I would still be willing to accept this directory structure (with or without my previous comments added). I think it's good to carry a bit of a discussion for changes like these, so I'm giving some time for this exactly 🙂

Also you caught us right when we were about to ship some changes we've been working on for a few weeks, so I have a feeling you might have to get the PR caught up with master. Sorry about that, but thank you for being open for the discussion 😃

karnett commented 4 years ago

FYI: I will get back to this soon.. had to update Xcode for job interview questions, so might be delayed a week or so.

karnett commented 4 years ago

don't have the time to get back to these and download the old Xcode again. Maybe someone can cherry-pick it and pick it up.