-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
Looking at this now. |
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 is an excellent initial implementation of the settings page that handles more of the embed map state management than I was expecting (which is a good thing)
In addition to a few small inline comments I have two medium-size suggestions:
We may want to release other features to production before the embedded map feature is complete, especially since embedded map has additional stakeholders beyond the OAR team. Based on that, we should consider creating a new switch (feature flag) that would hide the embed tab on the settings page.
When you are logged in, and you view the facility details for a one of the facilities that you have contributed, the profile link goes to an editable version of the profile form. It may be confusing to have a profile edit page that has different options depending on how it was navigated to. With this relatively small tweak to your implementation, we can have all editing take place on /settings
diff --git a/src/app/src/components/Settings.jsx b/src/app/src/components/Settings.jsx
index 806c71a..ab5ff1f 100644
--- a/src/app/src/components/Settings.jsx
+++ b/src/app/src/components/Settings.jsx
@@ -32,7 +32,7 @@ function Settings({ user }) {
</AppGrid>
<AppOverflow>
{user && activeTabIndex === 0 && (
- <UserProfile id={user.id.toString()} />
+ <UserProfile allowEdits id={user.id.toString()} />
)}
{activeTabIndex === 1 && <EmbeddedMapConfig />}
{activeTabIndex === 2 && (
diff --git a/src/app/src/components/UserProfile.jsx b/src/app/src/components/UserProfile.jsx
index 14ecd27..055dbda 100644
--- a/src/app/src/components/UserProfile.jsx
+++ b/src/app/src/components/UserProfile.jsx
@@ -1,6 +1,7 @@
import React, { Component } from 'react';
import { arrayOf, bool, func, string } from 'prop-types';
import { connect } from 'react-redux';
+import { Link } from 'react-router-dom';
import Grid from '@material-ui/core/Grid';
import CircularProgress from '@material-ui/core/CircularProgress';
import { toast } from 'react-toastify';
@@ -119,6 +120,7 @@ class UserProfile extends Component {
submitFormOnEnterKeyPress,
errorFetchingProfile,
id,
+ allowEdits,
} = this.props;
if (fetching) {
@@ -133,8 +135,9 @@ class UserProfile extends Component {
return <RouteNotFound />;
}
- const isEditableProfile =
+ const isCurrentUsersProfile =
user && [profile.id, Number(id)].every(val => val === user.id);
+ const isEditableProfile = allowEdits && isCurrentUsersProfile;
const profileInputs = profileFormFields
// Only show the name field on the profile page of the current user.
@@ -177,6 +180,13 @@ class UserProfile extends Component {
</React.Fragment>
);
+ const toolbar =
+ isCurrentUsersProfile && !allowEdits ? (
+ <Link to="/settings" href="/settings">
+ Edit
+ </Link>
+ ) : null;
+
const showErrorMessages =
isEditableProfile &&
errorsUpdatingProfile &&
@@ -229,6 +239,7 @@ class UserProfile extends Component {
<AppOverflow>
<AppGrid title={title} style={profileStyles.appGridContainer}>
<Grid item xs={12} sm={7}>
+ {toolbar}
{profileInputs}
{facilityLists}
{errorMessages}
const allSelected = !fields.some(f => !f.included); | ||
const someSelected = fields.some(f => f.included) && !allSelected; |
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 like the design of giving names to these boolean expressions so that conditional code below is more immediately understandable.
ab4bcdb
to
d5fe0ce
Compare
Made a number of updates - the largest being adding a feature flag for embedded maps. In order to use the feature flag with the tabs, I needed to move the tabs into a dynamically calculated array and grab the indexes from it, which in terms of future adaptability may be preferable regardless. I have a note out to Katie re: the help text. |
Looking at this now. |
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 tested the revisions and they work perfectly. Thanks.
Allows the user to access their profile, tokens, and embedded map configuration under tabs in a new settings section. Adds the initial embedded map configuration controls and preview.
e55a295
to
bf022d3
Compare
Overview
Allows the user to access their profile, tokens, and embedded map
configuration under tabs in a new settings section. Adds the initial
embedded map configuration controls and preview.
Follow-up Tasks:
Connects #1287
Demo
Testing Instructions
Checklist
fixup!
commits have been squashed