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

Small fixes #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Small fixes #36

wants to merge 3 commits into from

Conversation

oxanayoxana
Copy link
Contributor

  1. Updated Slack link in readme
  2. There was a problem while trying to edit your own project - fixed it
  3. In navbar 'Sign in with Github ' link was broken, because now you can not just sign, you have to chose your role. Don't know how to fix it properly - any ideas? For now I just made a redirect to the root page, so that it doesn't throw error.

Copy link

@denys-medynskyi denys-medynskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out my feedback. Let me know anything wrong/ not clear about them

def repo
@repos.map(&:name)
def fetch_repo
@repo = @repos.map(&:name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method returns an array of names, right?

Why does it called in singular fetch_repo?

end

def repos
def fetch_repos
@client = Octokit::Client.new

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move all these 3 lines in 1 ServiceObject?

For example in the end we would get the following:

class FetchRepo
def call
# client / github user fetched
client.repos(@github_user.login, query: { type: 'owner', sort: 'asc' })
end
end

And in the controller:

@repos = FetchRepos.call

What do you think about it?

Copy link
Contributor Author

@oxanayoxana oxanayoxana Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deny7ko look, I've managed to do that! ) But why the changes don't show up hmm...

end

def edit; end

def create
@project = Project.new(project_params)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some variables are initalized inside the method, some of them outside?
Keep one style:

@project =
@repos =

Instead of doing a mix

@project =
fetch_repos

It will make it easier to read your code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants