Conversation
| .wrapper { | ||
| text-align: center; | ||
| } | ||
|
|
||
| h1{ | ||
| display: inline-block; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you use the css naming convention outlined in the readme? These styles will clash with styles throughout the rest of the app
| <h1>Groups You Can Join!</h1> | ||
| </div> | ||
| <div className="HomePage__group-cards-container"> | ||
| <span className="HomePage__groupcard-1"> |
There was a problem hiding this comment.
You should never have a number in a class - by definition, classes should apply to multiple elements (as opposed to IDs)
Also, make sure your names are consistent (i.e. HomePage__group-cards v. HomePage__groupcard)
| </div> | ||
| <div className="CommunityPage_body"> | ||
| <div> | ||
| <header className="CommunityPage__header">Group Location:</header> |
There was a problem hiding this comment.
A header tag is a block element, not a text element, so your text here should be surrounded by some appropriate text tag (h3, p, etc)
| <Pill | ||
| className="Pill" | ||
| text={"Traveling"} | ||
| size={"wide"} | ||
| color={"purple"} | ||
| icon={"white"} | ||
| onClick={pillClick} | ||
| selectId={0} |
There was a problem hiding this comment.
The data for these pills should be populated from somewhere in the API
| <Pill | ||
| className="Pill" | ||
| text={"Traveling"} | ||
| size={"wide"} | ||
| color={"pink"} | ||
| icon={"white"} | ||
| onClick={pillClick} | ||
| selectId={0} | ||
| />{" "} | ||
| <Pill | ||
| className="Pill" | ||
| text={"Traveling"} | ||
| size={"wide"} | ||
| color={"pink"} |
There was a problem hiding this comment.
Lot of repetition here - can you replace this with a loop?
| let memberLength = 1; | ||
| if (memberLength){ | ||
| const memberLength = group.members.length; | ||
| } |
There was a problem hiding this comment.
This is odd - you're assigning memberLength a truthy value and then testing if it's truthy immediately afterwords
There was a problem hiding this comment.
Then recreating the variable
|
|
||
| <div className="MemberProfilePage__group-div-Her"> | ||
| <GroupsList heading="Her Groups" moreGroups="View All" to="/" /> | ||
| <GroupsList heading="Your Groups" moreGroups="View All" to="/" /> |
There was a problem hiding this comment.
Is this someone from the mock?
0xZakk
left a comment
0xZakk
left a comment
There was a problem hiding this comment.
Spend some time putting this in context for us
2 participants
Related Issue (include '#'): As a user I would like to be able to see the list of groups I can join #435
Description of changes made: include a new page which lists the groups
Is the feature complete/bug resolved/etc..: Would like to add a banner?
Any known bugs/strange behavior: none at the moment
Is there specific feedback you would like on these changes:
Screenshot(s):