Skip to content
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

Import definitions from usfs #8149

Merged
merged 5 commits into from
Aug 10, 2018
Merged

Conversation

annekainicUSDS
Copy link
Contributor

@annekainicUSDS annekainicUSDS commented Aug 9, 2018

We're moving some of the definitions that are pretty specific and potentially not generally usable back into vets-website. This is the first pass at doing that.

Accompanying PR on USFS: usds/us-forms-system#214

@va-bot va-bot temporarily deployed to vetsgov-pr-8149 August 9, 2018 19:42 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-8149 August 9, 2018 19:46 Inactive
@va-bot va-bot temporarily deployed to master/import-definitions-from-usfs August 9, 2018 20:06 Inactive
@annekainicUSDS annekainicUSDS changed the title WIP Import definitions from usfs Import definitions from usfs Aug 9, 2018
Copy link
Contributor

@cehsu cehsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some references in src/platform/forms/pages/applicantInformation.jsx should be updated as well.

import environment from '../../../platform/utilities/environment';

import * as address from 'us-forms-system/lib/js/definitions/address';
import fullNameUI from 'us-forms-system/lib/js/definitions/fullName';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we're missing a replacement of us-forms-system/lib/js/definitions/fullName and us-forms-system/lib/js/definitions/personId in src/platform/forms/pages/applicantInformation.jsx?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks, I missed those!

Copy link

@rianfowler rianfowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I was able to go t through the form 1900 locally without any issues and I only found one thing that wasn't swapped out (Claire noted it already).

@va-bot va-bot temporarily deployed to vetsgov-pr-8149 August 10, 2018 13:23 Inactive
@annekainicUSDS
Copy link
Contributor Author

@cehsu @rianfowler for some reason I'm now getting an issue where it's saying that the environment variable isn't defined in src/applications/edu-benefits/complaint-tool/helpers.js. I'm stumped on how my changes might have caused this issue. Do either of you have an idea?

@annekainicUSDS
Copy link
Contributor Author

Looks like that lint error is happening on master as well, so perhaps it was introduced by a different PR?

@rianfowler
Copy link

@annekainicUSDS We had a merge collision that was resolved improperly. I've just merged the fix- you should now be able to pull master and successfully build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants