Skip to content

Display groups im in#655

Closed
WRNoble wants to merge 9 commits intodevelopfrom
display-groups-im-in
Closed

Display groups im in#655
WRNoble wants to merge 9 commits intodevelopfrom
display-groups-im-in

Conversation

@WRNoble
Copy link
Copy Markdown

@WRNoble WRNoble commented Jul 8, 2020

  • Related Issue (include '#'): As a user I would like see what groups I am in #441

  • Description of changes made:

  • Is the feature complete/bug resolved/etc..:

  • Any known bugs/strange behavior:

  • Is there specific feedback you would like on these changes:

  • Screenshot(s):

@WRNoble WRNoble requested review from 0xZakk and Underwaterr July 8, 2020 19:07
Copy link
Copy Markdown
Contributor

@0xZakk 0xZakk left a comment

Choose a reason for hiding this comment

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

@WRNoble Looks good! See my feedback

Comment thread Pipfile
Comment thread client/src/App.js
import { far } from "@fortawesome/free-regular-svg-icons";
import { BASE_URL } from "./services/constants";
import { handleSignup, handleLogin, handleLogout } from "./services/User";
import GroupPage from "./components/GroupPage/GroupPage"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you using this page somewhere? Should there be a route for it?

Comment on lines +1 to +7
.wrapper {
text-align: center;
}

h1{
display: inline-block;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you update the CSS here so it's inline with the conventions in the README?

@@ -0,0 +1,47 @@
import React from 'react';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this page in the components directory and not the pages directory?

Comment on lines +27 to +32
<GroupCard
name="DIY with your kids"
members="98"
location="Boston, MA"
picture={card1}
/>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these be coming from the API?

Comment on lines +33 to +43
componentDidMount() {
axios.get(`${BASE_URL}/groups`)
.then(response => {
this.setState({
groups: response.data
})
})
.catch(error => {
console.log('Error while fetching and parsing API request', error);
})
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you put this in the appropriate service in the services/ directory? All AJAX requests should be in a service

}

render() {
console.log(this.state)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Be sure to delete all commented out code and any console.log or print statements before making a PR going forward

members="5"
location="Chicago, IL"
picture={picture2}
<div className="CommunityPage__group-location-container">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We decided to remove this Pill section right?

<Card outline size="medium-wide-directory">
<a href="/">Discover New Groups</a>
</Card>{" "}
<a className="seeMore-Button">See More</a>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you use names inline with the naming convention described in the README?

</div>
<div className="CommunityPage__groups-in-city-container">
{groups.map(group =>{
console.log(group)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Delete

@secondaryfun
Copy link
Copy Markdown
Contributor

The updates from this branch have been incorporated into #654 . This branch can be closed without merging.

@0xZakk 0xZakk closed this Jul 15, 2020
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.

3 participants