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

Allow users to join and leave pursuances. #227

Merged
merged 7 commits into from
Jul 13, 2018

Conversation

Moe-Shoman
Copy link
Collaborator

Closes #213


export const delMembership = membership => ({
type: 'DELETE_MEMBERSHIP',
payload: delMembershipReq(membership)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Moe-Shoman Please rename to deleteMembershipReq, just to follow the convention we've been using

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

payload: getMembershipsReq(filterOption)
});

export const delMembership = membership => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with delMembership -> deleteMembership


export const getMembershipsReq = filterOption => {
const filterKey = Object.keys(filterOption)[0];
const filterVal = Object.values(filterOption)[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

@Moe-Shoman You can do filterVal = filterOption[filterKey] here

const filterKey = Object.keys(filterOption)[0];
const filterVal = Object.values(filterOption)[0];
return postgrest
.getJSON('/memberships')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do ".getJSON(/memberships?${filterKey}=eq.${filterVal})" and have the database do the filtering for you :-D

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you don't need to do the filtering below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice! will do


case 'DELETE_MEMBERSHIP_FULFILLED':
const {
[String(action.payload.pursuance_id)]: _,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, what does the _ do/mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It signifies that the variable(Deleted membership) will not be used.

if (window.browser === 'Firefox') {
delete scrollIntoViewOptions.block;
scrollIntoViewOptions.block = 'end';
Copy link
Contributor

Choose a reason for hiding this comment

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

@Moe-Shoman Mind removing the scrollIntoView shtuff from this PR so we can take care of it elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@@ -13,7 +13,7 @@ const sortBySuggest = (suggest1, suggest2) => {
return suggest1.suggestionName.localeCompare(suggest2.suggestionName);
};

export const scrollIntoViewOptions = { behavior: 'instant', block: 'nearest' };
export const scrollIntoViewOptions = { behavior: 'smooth', block: 'nearest' };
Copy link
Contributor

Choose a reason for hiding this comment

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

@Moe-Shoman Remove this, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I believe this might have been a part of the solution for the Firefox/Chromium issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Switch this back to instant? Maybe smooth is more cross-browser compatible but I'm recording the demo in Chrome and instant is fine for today

Copy link
Contributor

Choose a reason for hiding this comment

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

Great; recorded this possible solution at #161 (comment) 👍

src/App.js Outdated
contributionPoints={contributionPoints}
username={username}
onRemoveNotification={removeNotification}
onIncreaseContributionAmount={increaseContributionAmount}
Copy link
Contributor

Choose a reason for hiding this comment

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

@Moe-Shoman This is normally a great idea, but I'm about to merge in code which removes this and reduxifies NavBar, so to save me some merge conflict headaches, can you convert this back to having all the annoying this.props.s in there and use return this.props.user.authenticated ? ... : ... below rather than the naked authenticated, please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, NP

return {
[membership.pursuance_id]: membership
};
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This prevents more than 1 person being a member of a pursuance, but let's not worry about this for today -- it's demo time :-D .

Copy link
Contributor

Choose a reason for hiding this comment

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

(at least from Redux's perspective it does)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I was using that as a ref to the users memberships

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not the memberships of the whole pursuance

Copy link
Contributor

Choose a reason for hiding this comment

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

Soon lets make each key ${membership.pursuance_id}_${membership.user_username} and the value an array

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh I see. I haven't thought this through well enough other than to say that it seems like it will soon make sense to have the users listed in AssignerSuggestions populated based on memberships.

But we can table this.

return membershipsArr.map((membership) => {
const pursuance = pursuances[membership.pursuance_id];
if (!pursuance) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice 👌

<p><strong>Mission:</strong> {pursuance.mission}</p>
</div>
</div>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unindent 2 spaces

Copy link
Contributor

Choose a reason for hiding this comment

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

after the second return

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unindent the top portion of this div, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

The return ( ... ) lines are indented 2 spaces too far

</div>
<PursuanceList />
{ user.authenticated && <PursuanceList />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Eeeexcellent

@elimisteve elimisteve merged commit 5526636 into PursuanceProject:kickstarter_demo Jul 13, 2018
@elimisteve
Copy link
Contributor

Merged! 🎉

(I merged locally then resolved merge conflicts, so GitHub's UI isn't saying it was already merged, but... it has been :-D )

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.

2 participants