Skip to content
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

added button images for github and other services #152

Closed
wants to merge 1 commit into from
Closed

added button images for github and other services #152

wants to merge 1 commit into from

Conversation

SoldierCoder
Copy link

No description provided.

@PragTob
Copy link
Member

PragTob commented Jul 30, 2013

Hi there,

what are these button images for? This PR does not integrate them anywhere on the site as far as I can see
. I'm suspecting that they will be used for various logins. If that's the case I prefered it if they were added within a pull request that adds this functionality as they do not provide any value of their own.

Cheers,

Tobi

@SoldierCoder
Copy link
Author

"Other than this I'd like to encourage you to open Pull Requests as early
as possible so we can have a look at them. They don't even have to be ready
to be merged yet, a start to get feedback is enough" -- Tobias Pfeiffer

Tobias, that was merely a demonstration of pushing images up to the repo.
They will be used, however, as part of feature that allows users to login
with omniauthable services See issue #8. But since you've brought it up,
do you mind if we add things in steps? or does everything have to be
included all together? Based on what you said earlier, I thought it would
be ok.

Cheers,

Ed

On Tue, Jul 30, 2013 at 12:30 PM, Tobias Pfeiffer
[email protected]:

Hi there,

what are these button images for? This PR does not integrate them anywhere
on the site as far as I can see
. I'm suspecting that they will be used for various logins. If that's the
case I prefered it if they were added within a pull request that adds this
functionality as they do not provide any value of their own.

Cheers,

Tobi


Reply to this email directly or view it on GitHubhttps://github.com//pull/152#issuecomment-21803796
.

@PragTob
Copy link
Member

PragTob commented Jul 31, 2013

Hello Ed,

I think there might have been a misunderstanding. Let me try to clear that up :-)

Yes I encourage you to do Pull Requests early on. With early on I meant for something that provides value. So what do I think of as providing value?

  • A feature (e.g. new functionality for the end user)
  • increased external quality (faster responses, better design... whatsoever)
  • increased internal quality (refactoring, more tests, gem for code metrics etc.)

I wrote early to highlight that none of these have to be completely done in order to start a pull request e.g.:

  • feature is done, but tests are missing
  • basics of feature are done, but some edge cases/nice ui other things are not working properly
  • OAuth works, but just for github for instance (there can be separate PRs for every provider)
  • someone just started to implement a feature and feels unsure, if this is the right place to put the code/right approach and needs feedback

We can then discuss this and further commits can be added to the Pull Request by pushing to the branch where the pull request is coming from. In that way Pull Requests may be incrementally refined via early feedback.

Also I wanted to encourage opening Pull requests for seemingly small things, such as:

  • Adding a tool that helps with development (as I mentioned before, the addition of simplecov would definitely fall into that category) - improves internal quality
  • Refactoring a bit of code - improves internal quality
  • Deleting unused files - increases internal quality
  • new tests for previously untested behaviour - increases internal quality
  • the tiniest feature such as a link to a frequently used site that has not been there previously - new feature (kind of ;-) )
  • design changes that make it look better/whatsoever - increases external quality

However I do not want to include additions in steps e.g. merging 5 Pull Requests for one feature. The commit with the images here could be part of a pull request for the Oauth feature. However they alone would not be enough to start that PR since there already should be some meaningful code to discuss/improve.

The images here provide no value of their own and even if they were inserted in the web app would not really provide value as they would be used to sign in with different services and that functionality is still missing.

So I'm gonna go ahead and close this PR. I hope it is clearer now what I mean with early pull requests. I think this is good because one big pull request at one point is a lot and very difficult to review. Plus there might be things that we want to have changed that are way more difficult to change later on if code already builds upon them. Also early and updated PRs reduce the risk of merge conflicts.

I will also send this in an email for clarifications. If you have any further questions, please feel free to ask.

Cheers,
Tobi

@PragTob PragTob closed this Jul 31, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants