-
Notifications
You must be signed in to change notification settings - Fork 59
Improve save user locations #280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, but I do have a couple of changes. We do have some updated mocks that deal with the user account page (see sprint review presentation) that this doesn't align with, but perhaps we should discuss further (maybe we can handle alignment in a separate PR).
refactor(LocationField): refactor HOC for LocationField
Encapsulate code to connect LocationField to redux store
…eLocation Also move FavoriteLocationField and FavoriteLocationDropdown to favorite-location-controls
| */ | ||
| function setUser (user, fetchTrips) { | ||
| return function (dispatch, getState) { | ||
| positionHomeAndWorkFirst(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think the client should be concerned with this, but I won't make this blocking so we can move forward. I do think we should add an issue to otp-middleware that handles this sorting before persisting an update and ensures that there is only one home/work location for any given user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted in ibi-group/otp-middleware#136.
| /> | ||
| <Route | ||
| component={FavoritePlaceScreen} | ||
| path={`${PLACES_PATH}/:id`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating a place in the context of a new account, I think we should avoid placing the My account header content at the top of the page. Doing so takes the user out of the create account context and also shows the Trips/Settings links, which should not be available at this stage. I don't want to overcomplicate this though... thoughts on a clean approach here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one main thing: the 'My account' page header should not show when adding a place while creating account. Perhaps we could use a path like account/create/places/0.
|
Alright, ready for review again... |
| locationType='to' | ||
| onLocationSelected={this._handleLocationChange} | ||
| showClearButton={false} | ||
| static={isMobile()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigh... I forgot that showing the field in mobile view is why we opted for the buttons above instead of below. We may want to change this so that in mobile mode the buttons render at the top of the screen and in desktop they're below the location field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add that. The other thing we could do in mobile (in another PR) is to flush the back/save buttons to the bottom of the screen so that they are always shown, but that may be also be tricky...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |

This PR changes the user's saved locations pane
usingby updating the widget (LocationFieldfor entering locations and obtaining the locations coordinates, so that coordinates are persisted with the user preferences.FavoritePlaceRow) to display each location (place).The PR also lets the userClicking aFavoritePlaceRowopens a page (FavoritePlacePage) where the user can change the type of location (e.g. 'dining', 'custom' - what other predefined types need to be added?).A dropdown is added to the left of the location. (It is not possible to edit the nickname of each location yet.)Code-wise, each location row is now its own
FavoriteLocationFavoritePlaceRowcomponent. TheFavoritePlacePagethathosts a styledFavoriteLocationFieldPlaceLocationFieldversion ofLocationField.This will allow further refinements of the layout or allowing more advanced editing so let me know if you have any suggestions.EDIT 1:
LocationFieldinstances to the redux store.Icon(from narrative).EDIT 2:
FixedFavoriteLocationcomponent and removed quirks in the code regarding placeholdersfavorite-location-controls.EDIT 3
/user/favorite-locations/EDIT 4
/user/favorite-locations/=>/user/placesand renamed components within.EDIT 5:
<Link>and<LinkContainer>that preserve the query parameters (fix Unable to use LinkContainer #202)./account/create/terms), so that when user finishes editing a place, they remain in the wizard mode instead of seeing a single page with all settings.EDIT 6:
When editing places:
Screenshot:NEW screenshots:NEWEST screenshots