Conversation
Need updated API settings.
…tform/CoTrip into community-group-page-detail
0xZakk
left a comment
0xZakk
left a comment
There was a problem hiding this comment.
@bavjean This is a great start! See my comments to your code, as well as my notes below.
This is what I see when I go to create a group:
There are a couple of things you can do to really improve this form:
- Put a max-width on the container or wrap it somehow. Use your best judgement, but it probably shouldn't be much wider than like 500px.
- Put some space between the inputs
- Add labels to the inputs (in addition to the placeholder text)
- Give the form a title
- Make the button more visible. Buttons shouldn't be full width, instead have it be the width of the text with some padding and have it justified to the left.
This is a great resource on building forms: https://www.carbondesignsystem.com/patterns/forms-pattern
| <div> | ||
| <header className="CommunityPage__header"> | ||
| Groups in WASHINGTON, DC: <a href="./view-group"> View Sample Page </a> | ||
| Groups in WASHINGTON, DC: <a href="./view-group"> View Sample Page </a><a href="./create-group">Create Group</a> |
There was a problem hiding this comment.
You can remove the sample page link, since the group pages should now link to groups!
| submit = () => { | ||
| console.log(this.state); | ||
| axios({ | ||
| method: 'POST', | ||
| url: `${BASE_URL}/groups`, | ||
| data: { | ||
| title: this.state.title, | ||
| description: this.state.description, | ||
| location: this.state.location | ||
| } | ||
| }) | ||
| }; |
There was a problem hiding this comment.
All ajax requests should go in a service in the services/ directory
| getUSStates = () => { | ||
| axios.get(`${BASE_URL}/location/states`).then(res => { | ||
| console.log(res.data); | ||
| this.setState({ USStates: res.data }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This should also be in a service
| getCitiesFromState = event => { | ||
| if (event.target.textContent === "") this.setState({ stateFound: false }); | ||
|
|
||
| let USStates = this.state.USStates; | ||
|
|
||
| //TextContent because materialUI outputs an <li> tag | ||
| let userSubmission = event.target.textContent.toLowerCase(); | ||
|
|
||
| USStates.forEach(state => { | ||
| if (state.name.toLowerCase() === userSubmission) { | ||
| axios.get(`${BASE_URL}/location/bystate?state__code=${state.code}`).then(res => { | ||
| this.setState({ | ||
| stateFound: true, | ||
| currentStateCities: res.data | ||
| }); | ||
| }); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This is going to be a lot of ajax requests - maybe we can load the cities once the state is selected, like we do on the account creation flow?
There was a problem hiding this comment.
I used the account creation flow to do this part. I thought that with the conditional there it will wait to find a match for a valid state before loading the cities.
|
|
||
| let userSubmission = event.target.textContent.toLowerCase(); | ||
|
|
||
| // for(let i=0;i<currentStateCities.length) |
There was a problem hiding this comment.
Always delete commented out code!
|
|
||
| setLocationID = event => { | ||
| let currentStateCities = this.state.currentStateCities; | ||
| // console.log(currentStateCities); |
There was a problem hiding this comment.
Delete commented out code and always remove any console.log statements
| binarySearch = (cities, userSubmission, start, end) => { | ||
| // Base Condition | ||
| if (start > end) return false; | ||
|
|
||
| // Find the middle index | ||
| let mid = Math.floor((start + end) / 2); | ||
|
|
||
| // Compare mid with given key x | ||
| if (cities[mid].name.toLowerCase() === userSubmission) | ||
| return { found: true, location: cities[mid] }; | ||
|
|
||
| // If element at mid is greater than x, | ||
| // search in the left half of mid | ||
| if (cities[mid].name.toLowerCase() > userSubmission) | ||
| return this.binarySearch(cities, userSubmission, start, mid - 1); | ||
| // If element at mid is smaller than x, | ||
| // search in the right half of mid | ||
| else return this.binarySearch(cities, userSubmission, mid + 1, end); | ||
| }; |
There was a problem hiding this comment.
There has to be a better way to do this.
There was a problem hiding this comment.
This is the only part I have not had time to mess with yet, this also came from the account creation flow.
| return ( | ||
| <div className="CommunityPage"> | ||
| <Banner background={Banner__Community}> | ||
| <div className="community-page-header"> |
There was a problem hiding this comment.
use the CSS naming convention in the documentation
There was a problem hiding this comment.
*For all your CSS class names
4 participants
Related Issue (include '#'): As a user I would like to be able to be able to create a group based on a shared interest and location #428
Description of changes made:
Made a new community page for creating a new group
Is the feature complete/bug resolved/etc..:
Still not entirely finished, need to redirect user after creation, and should probably add checker to make sure title and/or description are not empty.
Probably want to redirect to the view-group page but don't think that has been implemented yet.
Could use some css, and a better button to link to the page.(unsure best place to put it, so I just stuck it in the community join group page)
Any known bugs/strange behavior:
Is there specific feedback you would like on these changes
I still need to add the things I mentioned above, mostly just wanted to get some feedback in case I need to change anything major.
I was originally using the form from the onboarding page(FilterSettingsForm) since it was exactly what I was trying to do, but realized the back end does not support interests within the group model. After considering changing the model, I decided it wouldn't make a whole lot of sense to include interests with this since the location and title should be enough to determine if the user wants to join.