Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

Fundraiser login & ActionForm bug fix #1096

Merged
merged 3 commits into from Jan 31, 2018
Merged

Fundraiser login & ActionForm bug fix #1096

merged 3 commits into from Jan 31, 2018

Conversation

rodrei
Copy link
Contributor

@rodrei rodrei commented Jan 31, 2018

  • Added login_member action to fundraiser store. This allows the member information to be set after the component has been initialized.

  • Modified action_form.js to publish the form data after being submitted.

  • Fixed bug that caused action_form to submit wrong values. The action form prefilling logic was broken, and was actually modifying the DOM in a wrong way (specifically checkboxes and radio buttons). The radio buttons, check boxes and textareas prefilling doesn't work (and wasn't working before either).

@@ -88,3 +88,5 @@ window.mountFundraiser = function(root: string, data: MountFundraiserOptions) {
});
}
};

window.storeDispatch = dispatch;
Copy link
Member

Choose a reason for hiding this comment

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

Had you intended on keeping this global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I need to expose the store to our petition backbone app. Is there any other way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Left a comment on the other PR: the redux store is in window.champaign.store so you'll have access to store.dispatch(), store.getState(), store.subscribe(), etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, I'll change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the store was already in window.champaign.store. Changing the code to use that.

prefill() {
const data = {};
for (const field of this.props.fields) {
data[field.name] =
this.getPrefillValue(field.name) || field.default_value;
data[field.name] = this.props.formValues[field.name]
Copy link
Contributor

@vincemtnz vincemtnz Jan 31, 2018

Choose a reason for hiding this comment

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

Did you mean this.props.formValues ? ... : ... here? It seems from the former getPrefillValue that the prop might be undefined / null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch! good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's already there! I was confused. The ternary operator expands to multiple lines, check the following 2 lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but the first line tries to access a property (but the object could be undefined).

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I'm not sure if it's intended or just being overprotective? formValues is always an object in the reducer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally the store should make sure that formValues is always an object, but right now it's not doing that. I'll add the check.

@@ -159,15 +154,15 @@ export class MemberDetailsForm extends Component {
onSubmit={this.submit.bind(this)}
className="form--big action-form"
>
{this.props.fields.map((field, ii) =>
{this.props.fields.map((field, ii) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like ii is unused here (although that was also there from the previous version, but worth cleaning up)

@@ -130,12 +132,13 @@ export class FundraiserView extends Component {
}
fields={fields}
outstandingFields={outstandingFields}
prefillValues={formValues}
formValues={formValues}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@rodrei rodrei merged commit e46d9aa into development Jan 31, 2018
@rodrei rodrei deleted the fundraiser-login branch January 31, 2018 17:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants