Skip to content
This repository was archived by the owner on Nov 26, 2024. It is now read-only.

Conversation

@larafa
Copy link
Contributor

@larafa larafa commented Mar 26, 2018

Still need to figure out the db relationships for followers and followings

}

public function following()
public function followings()
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure you refactor the places where this function is being called too.

$isFollowing = $requestorUser && $requestorUser->following->contains($user);

And here

if (!$this->following->contains($user_id)) {

@@ -0,0 +1,57 @@
import React, { Component } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could refactor FollowersList.js and FollowingsList.js into 1 component since they are so similar and just pass different data into them, kinda like what we do with Card.js

return (
<div
key={index}
className="follower col-lg-3 col-md-5 m-2 animated fadeInUp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think follower is a css class. If you were looking at some of the card code. card is Actually a bootstrap class.
https://getbootstrap.com/docs/4.0/components/card/

You can also use reactstrap to use Card as an element instead of a class
https://reactstrap.github.io/components/card/

</Link>
</div>
</div>
<div className="follower-body">
Copy link
Contributor

Choose a reason for hiding this comment

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

follower-body is not a class

</div>
</div>
<div className="follower-body">
<h5 className="follower-title text-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

follower-title is not a class

{/*Routes that only logged in Users can access*/}
<UserRoute path="/useronly" component={Debug} />
<UserRoute path="/account" component={AccountPage} />
<UserRoute path="/followers/:id" component={FollowersPage} />
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are only going to display the followers and followings on the Account page, I don't think we need routes.

@codecov-io
Copy link

codecov-io commented Mar 30, 2018

Codecov Report

Merging #69 into master will decrease coverage by 0.46%.
The diff coverage is 11.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #69      +/-   ##
============================================
- Coverage     23.58%   23.11%   -0.47%     
- Complexity      133      135       +2     
============================================
  Files            91       92       +1     
  Lines          1463     1510      +47     
  Branches        188      193       +5     
============================================
+ Hits            345      349       +4     
- Misses         1042     1085      +43     
  Partials         76       76
Impacted Files Coverage Δ Complexity Δ
frontend/src/components/FollowList.js 0% <0%> (ø) 0 <0> (?)
frontend/src/reducers/reducer_user.js 0% <0%> (ø) 0 <0> (ø) ⬇️
frontend/src/pages/Account.js 4.34% <0%> (-0.2%) 0 <0> (ø)
backend/app/Http/Controllers/ProfileController.php 68.33% <14.28%> (-7.6%) 22 <2> (+2)
frontend/src/actions/users.js 12.12% <15.38%> (+0.61%) 0 <0> (ø) ⬇️
backend/app/Models/User.php 70.58% <50%> (ø) 12 <1> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41bac36...0ceeadd. Read the comment docs.

bbemis017
bbemis017 previously approved these changes Apr 4, 2018
Copy link
Contributor

@bbemis017 bbemis017 left a comment

Choose a reason for hiding this comment

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

Everything looks good, but I don't think email.html was meant to be on this one.

@@ -0,0 +1,39 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant to have this file on your other branch?

Copy link
Contributor

@bbemis017 bbemis017 left a comment

Choose a reason for hiding this comment

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

🚢

@larafa larafa merged commit b3f0d61 into master Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants