fabacab / multisite-directory

:card_index::globe_with_meridians: Add a browseable, flexible directory of the sites in a WP Multisite network.
https://wordpress.org/plugins/multisite-directory/
GNU General Public License v2.0
1 stars 6 forks source link

Add category fields to the network admin new site form #19

Closed hvianna closed 7 years ago

hvianna commented 7 years ago
fabacab commented 7 years ago

Thanks for your contribution! :) I think this would be a nice addition to the project, and at first glance everything looks really clean and beautiful. I do have some concerns about the implementation you've proposed here.

I'm gonna go pull these changes down myself and give them a try on my test network. Very much appreciate your work on this so far and there's definitely a lot in here worth merging. 👍

hvianna commented 7 years ago

Thank you for the super positive response! I'm really glad this will actually be useful to the project! :D

I completely agree with you about the commented out parts, but I was a bit hesitant to simply delete your original code. As a bonus, TIL about code smell! :P

I'll go thru your recommendations and will try to fix everything tomorrow morning.

fabacab commented 7 years ago

This looks almost ready to merge now! Thanks again @hvianna! Last things to do before merging is to clean up your commit history. Since you've already got a few non-atomic commits, the easiest thing to do is just squash all of these together into one commit. Follow these instructions if you don't already know what that means.

  1. Make a topic branch so your current work (which seems to be on master) can be referenced somewhere other than master.
  2. Since you should be committing on top of develop, not master, checkout develop
  3. Now you can squash all your work into one single commit.
  4. And finally you should commit this.

The git(1) commands for this procedure might look like the following:

git branch my-pull-request master      # create a new branch called "my-pull-request" at the same place as "master" currently is
git checkout -b develop origin/develop # create a local develop branch that tracks the "develop" branch in your remote called "origin"
git merge --squash my-pull-request     # squash all commits present in "my-pull-request" into one commit
git commit -v                          # actually commit the single, squashed commit object prepared by the previous command

At this point your local develop branch will contain the contents you actually want to submit as your pull request, so you should git push and update this pull request's head branch to reference that. GitHub will then show only 1 commit object to merge and it will be a clean merge. I can then merge your pull request into my develop branch and from there merge it into my local master branch to update WordPress's Subversion side of things in a way that won't freak it out because we've been using Git all along and not Subversion like it expects.

Thanks again!

hvianna commented 7 years ago

I could not find how to change my branch to 'develop', so I opened a new pull request.

fabacab commented 7 years ago

ba97555b9aa8e6e36ef3628f165796ff41f57a60 has the best commit message ever. :)