Skip to content

Small fixes#36

Open
oxanayoxana wants to merge 3 commits intoarkency:masterfrom
oxanayoxana:master
Open

Small fixes#36
oxanayoxana wants to merge 3 commits intoarkency:masterfrom
oxanayoxana:master

Conversation

@oxanayoxana
Copy link
Copy Markdown
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
Copy Markdown

@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

Comment thread app/controllers/projects_controller.rb Outdated
def repo
@repos.map(&:name)
def fetch_repo
@repo = @repos.map(&:name)
Copy link
Copy Markdown

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?

Comment thread app/controllers/projects_controller.rb Outdated

def repos
def fetch_repos
@client = Octokit::Client.new
Copy link
Copy Markdown

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
Copy Markdown
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...

def edit; end

def create
@project = Project.new(project_params)
Copy link
Copy Markdown

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