-
Notifications
You must be signed in to change notification settings - Fork 126
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
Initial skeleton of GIBCT #4803
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.
This generally looks good! I left some notes on how to get the common store working; let me know if you have any questions or issues when trying to get it to work.
Also, AppContent
should be removed if users aren't logging in to use GIBCT
return null; | ||
} | ||
|
||
const headerDisplacementCSS = ` |
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.
It is possible to use these styles by having them in a class? instead of inline?
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.
Possible, but I wanted to make it explicit that I'm affecting rendering outside the visible boundaries of the component without actually touching DOM nodes outside the component. Not possible.
src/js/gi/containers/GiBillApp.jsx
Outdated
import Breadcrumbs from '../components/Breadcrumbs'; | ||
import AboutThisTool from '../components/AboutThisTool'; | ||
|
||
function AppContent({ children, isDataAvailable }) { |
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.
If veterans do not need to be registered to use this tool, we can get rid of the AppContent
wrapper. It is meant to be used in conjunction with a RequiredLoginView
component that wraps the entire thing (and calls AppContent after the user logs in successfully).
src/js/gi/containers/Modals.jsx
Outdated
renderProfilePageModals() { | ||
return ( | ||
<span> | ||
<Modal onClose={() => this.props.hideModal()} visible={!!this.props.giModals.retention}> |
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.
for these onClose function with no parameters, I believe you can do onClose={this.props.hideModal}
; no need to wrap it in a function
@@ -0,0 +1,8 @@ | |||
import { createStore, applyMiddleware, compose } from 'redux'; |
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.
We can use the common store for this instead of having a separate store for GIBCT
import { createHistory } from 'history'; | ||
import initReact from '../common/init-react'; | ||
import routes from './routes'; | ||
import { store } from './store'; |
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.
Use the common store import { commonStore } from '../common/store';
To allow it to work, you also need to go to common/reducers/index.js
and import your GIBCT reducers.
https://github.com/department-of-veterans-affairs/vets-website/blob/master/src/js/common/reducers/index.js
Then, in your GIBCT app, in the components that use state you can access them like so (depends on where you put GIBCT in the common store reducer):
This is an example from blue button
const mapStateToProps = (state) => {
const bbState = state.health.bb;
return {
form: bbState.form,
};
};
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.
Oh duh - that's what I was missing - updating common reducers. Thank you!
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.
As stated in other comments we are not going to be using commonStore.
Hey @ben-USDS-damman -- Do we need to use |
@crwallace Was under the impression we wanted to move over to |
We are trying to use the store for attributes that might need to be shared across various apps. Since GIBCT is a stand alone app it doesn't really need to be in the |
Nope. Thanks for clarifying. We won't use commonStore. |
b53325b
to
5a11479
Compare
744e37d
to
6cb0c46
Compare
Addresses previous comments by @bshyong. I'm in the middle of completing the profile page and will be pushing that tonight. |
Will review again after profile page and other changes are pushed
Please take special note of what I'm doing with |
src/js/gi/actions/index.js
Outdated
|
||
export function fetchConstants() { | ||
const previewVersion = ''; // TODO | ||
const url = `${api.url}/calculator/constants?preview=${previewVersion}`; |
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.
Adding a note to remember to remove the temporary hack in config.js
later on
|
||
|
||
/** | ||
* A form checkbox with a label that can display error messages. |
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.
Is it possible to use the common component for Errorable Checkbox here instead of a specific one for GIBCT?
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.
The Checkbox and RadioButtons components were copied here to make that possible in a future iteration, but my needs were simpler and significantly different such that I didn't want to tackle generalizing common components right away. (Aside from making it easy to diff to what the diffs are between the common and gibct components.) I thought about using inheritance but that seemed less practical and would've introduced a dependency on the common components that I'm not sure is a good idea.
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.
- the concepts of required fields and error messages do not apply to GIBCT
- click handlers on the common components also seemed like overkill and I frankly didn't understand why they were implemented the way they are, i.e.
{value: x, dirty: true}
. I'm sure there's a good reason but it felt like the reason was maybe only well-suited to more complicated forms and schema forms elsewhere on the site.
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.
For what it's worth, the value
and dirty
pattern is there to support showing validation errors only after a field has been interacted with (i.e. it's dirty). Definitely not useful if you don't have required or error messages.
Those components were originally built for HCA and forms like what the Rainbows team is building, but we're moving away from using these particular components now (the schema form work). So there's probably some room in the future to have a simpler set of components that fit more cleanly in apps that just have a few input fields.
|
||
import ToolTip from '../../common/components/form-elements/ToolTip'; | ||
import ExpandingGroup from '../../common/components/form-elements/ExpandingGroup'; | ||
|
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.
Is it possible to use the common Radio Button component?
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.
(The Checkbox and RadioButtons components were copied here to make that possible in a future iteration, but my needs were simpler and significantly different such that I didn't want to tackle generalizing common components right away. (Aside from making it easy to diff to what the diffs are between the common and gibct components.) I thought about using inheritance but that seemed less practical and would've introduced a dependency on the common components that I'm not sure is a good idea.)
|
||
const getEligibilityDetails = (state) => { | ||
const details = Object.assign({}, state.eligibility); | ||
delete details.dropdowns; |
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.
why is the dropdowns attribute deleted? no change needed, just wondering
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.
The use of reselect looks fine to me. Because some of the attribute calculations are fairly complicated, reselect lets us only recalculate when needed.
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.
details.dropdowns
holds the shown/not-shown state of the eligibility interdependent dropdowns and isn't used in the calculation. IOW the code for the visible/hidden presentation of the eligibility dropdowns is separate from the code that determines what chosen values become inputs to the calculation.
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.
My approach is to try to make inputs as explicit/obvious as possible - like I did for getRequiredAttributes
in the estimator.js
selector.
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 noticed a few places that are probably failing the lint and the use of certain components. Some minor things.
* `onValueChange` - a function with this prototype: (newValue) | ||
* `required` - boolean. Render marker indicating field is required. | ||
*/ | ||
class Checkbox extends React.Component { |
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.
Why create a duplicate class rather than just reuse the already existing ErrorableCheckbox? Seems virtually the same except for the renamed onValueChange
prop.
* `value` - string. Value of the select field. | ||
* `onValueChange` - a function with this prototype: (newValue) | ||
*/ | ||
class RadioButtons extends React.Component { |
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.
Similar question about this as the Checkbox
component. Seems like a copy of ErrorableRadioButtons
with a few changes. It seems like this is to avoid using field objects in the form of { value: <string>, dirty: <boolean> }
created by the makeField
function.
import { connect } from 'react-redux'; | ||
import * as actions from '../../actions'; | ||
|
||
import If from '../If'; |
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 think the linter will fail if you're not using things you import. In this case, doesn't seem like you're using If
.
}; | ||
|
||
const mapStateToProps = (state) => state; | ||
const mapDispatchToProps = (dispatch) => { |
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's a shorthand for this where you don't have to manually map each action's name to a dispatch-wrapped version of itself. Also, since you only needed exitPreviewMode
, you import it by itself.
import { exitPreviewMode } from '../../actions';
...
const mapDispatchToProps = {
exitPreviewMode
};
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.
Wow, nice of redux to make that work!
@@ -0,0 +1,24 @@ | |||
import React from 'react'; | |||
|
|||
class If extends React.Component { |
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.
Rather than create an If
component, I think it's more idiomatic to either (1) conditionally assign JSX to variables or (2) just inline the conditions. https://facebook.github.io/react/docs/conditional-rendering.html
// Conditionally assigning JSX.
let conditionalSection;
if (condition) {
conditionalSection = <div>This section appears because of condition.</div>;
}
return (
<div>
<span>This always appears.</span>
{conditionalSection}
</div>
);
// Inlining conditional sections.
function renderConditionalSection() {
return <div>This section appears because of condition.</div>;
}
function render() {
return (
<div>
<span>This always appears.</span>
{condition && renderConditionalSection()}
</div>
);
}
From this SO answer: "As for the If component you asked about, one problem it has is that in its current form it will evaluate its children even if the condition is false. This can lead to errors when the body of the If only makes sense if the condition is true."
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 seriously hate this If
component, but it was a necessity when porting over legacy code.
return ( | ||
<div className="search-result"> | ||
<div className="outer"> | ||
{this.cautionFlag.bind(this)()} |
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.
Usually we just bind every method in the constructor and don't have to deal with fixing references to this
later.
constructor(props) {
super(props);
this.cautionFlag = this.cautionFlag.bind(this);
this.place = this.place.bind(this);
this.renderEstimate = this.renderEstimate.bind(this);
}
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.
done
src/js/gi/config.jsx
Outdated
@@ -0,0 +1,21 @@ | |||
import environment from '../common/helpers/environment'; |
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 file doesn't need the jsx
suffix, since it doesn't use any React, so can it just be config.js
.
src/js/gi/reducers/autocomplete.js
Outdated
inProgress: false, | ||
previewVersion: null, | ||
search_term: '', | ||
facility_code: null, |
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.
Why are these two keys snake_case while the first two are camelCase? In any case, linter will fail on the snake_cased keys, so they should be changed to camelCase.
If it's because the data received from the API would need to be transformed into the proper casing, you can actually just add the header X-Key-Inflection: 'camel'
to the requests.
const getRequiredAttributes = (_state, props) => { | ||
const { type, bah } = props; | ||
return { type, bah }; | ||
}; |
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.
If the two above get*
functions are getting used in a few different modules, could they be in utils/helpers.js
and imported to where they're needed?
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.
These are truly private functions that will be different across selectors.
src/js/gi/selectors/estimator.js
Outdated
// The monthly benefit rate for non-chapter 33 benefits | ||
const isOJT = its.type === 'ojt'; | ||
const isFlight = its.type === 'flight'; | ||
const n = Number(your.number_of_dependents); |
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.
Here's a neat little shorthand for Number conversions:
const n = +your.number_of_dependents
.
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.
Nice!
Thanks for that info Jeff!
|
2618d03
to
63cdef3
Compare
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.
Left a few comments, but otherwise looks good. I notice similar things as my last review for things that would trip the linter. Be sure to run npm run lint
to capture all the errors to fix before the tests pass.
const expanded = this.state.expanded; | ||
return ( | ||
<li> | ||
<button onClick={this.toggle.bind(this)} className="usa-accordion-button" aria-expanded={expanded} aria-controls="amendment-1"> |
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.
Should be bound in the constructor.
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.
done
return ( | ||
<p> | ||
<strong>Veterans tuition policy: </strong> | ||
<a href={`http://${it.vet_tuition_policy_url}`} target="_blank">View policy</a> |
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.
Does it make sense to just always use https here, or is it just not available?
} | ||
} | ||
|
||
const mapStateToProps = (state, props) => state; |
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.
Typically if the component only needs a part of the state, you want to pare down the parts of the store you want to translate to the component's props. In this case, you're only going to be using profile.attributes
.
<tspan x="220" y={y}>{text}</tspan> | ||
</text> | ||
); | ||
const AverageMark = ({percent, text}) => { |
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.
Other than ValueBar
, these components don't directly use Graph
's props. They could probably be defined outside of the class, so that they don't get initialized on each render. Then for ValueBar
, this.props.max
could just be passed in as an arg.
src/js/gi/reducers/profile.js
Outdated
function normalizedAttributes(attributes) { | ||
const name = attributes.name ? attributes.name.toUpperCase() : attributes.name; | ||
const city = attributes.city ? attributes.city.toUpperCase() : attributes.city; | ||
const state = attributes.state ? attributes.state.toUpperCase() : attributes.state; |
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.
For these, you could just do something like:
const name = attributes.name && attributes.name.toUpperCase()
.
b4ffbf4
to
33cf021
Compare
5fad9fb
to
a97bc72
Compare
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.
LGTM. The main things that can be fixed after are moving away from commonStore
and usage of some components.
This code does NOT use commonStore at all. To be clear. |
Looks like the build is passing. @ben-USDS-damman do you still need to add profile updates or are those in? Since this unauthed can we triple check and make sure it is behind a feature toggle and not publicly visible unless you know the URL before we merge? |
It's in the ignoreList AND the rev proxy is sending traffic to the old rails app still anyway - something we need to change for dev/staging if we want to look at this there. |
Since I initially worked on the frontend we've planned design and UX changes. After talking it over with @saneshark on Friday I created a new clean branch off of master, built a skeleton based on the blue button and facility location code, and began layering on my previous work.
As I do this I'm cleaning it up and making sure it comports to the work @melwoodard and others have done on the new design and content. It's mostly copy-paste and then making any necessary changes to form/function.
Seeking feedback on the general code structure and also wondering how I can make this use the new site-wide commonStore instead of its own store. @crwallace? I tried to do it based on blue button but couldn't get it to work. #newb