-
Notifications
You must be signed in to change notification settings - Fork 2
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
add organization: redux and modal stages #29
Conversation
If your plan is to pull commits from other branches into this branch, let's just make one PR to handle this entire add organization feature. I feel like we're going to get some weird merge conflicts if we're picking commits from branches and making changes. How do you feel about that? |
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.
Hey!! This is awesome, great work!
A couple higher level things to give context to some of the inline comments.
- pull the two commits I made!!! Just added a lint script because i saw a few linter errors in the terminal. Run
npm run lint
and fix anything that you see please :). I may do a husky setup so we can't commit anything without running the lint command. - The loading, success, and error state modals for checking membership should be a single component, rendered outside of the
Organization
modal component. It's a good practice to not declare components inside of other components because when the outermost component re-renders, then all the inner components are forced to rerender when they don't necessarily need to. - We shouldn't be accessing anything directly off
boxes[ethereumAddress]
anymore. If you find yourself needing to do that, you can just update theuseProfile
hook.
Your react is getting noticeably stronger very quickly!!! keep up the great work.
Hey @chadoh this is a lot to review, big PR. We tried to make smaller ones at first, but doing so and waiting on reviews seemed to just create blockers. Would love to get you acquainted with this code base, so it could be cool for you and @rkzel to walk through the codes here if you both have the time. (I'd be happy to as well, whatever you both prefer). We could do a three way pair or something too. But do let me know if you're too busy to review, and we'll work on getting this in ASAP. |
src/client/components/styled-components/AddOrganizationError.js
Outdated
Show resolved
Hide resolved
src/client/components/styled-components/AddOrganizationSuccess.js
Outdated
Show resolved
Hide resolved
src/client/components/styled-components/AddOrganizationError.js
Outdated
Show resolved
Hide resolved
import React from 'react' | ||
import PropTypes from 'prop-types' | ||
|
||
const AnimationLoadingCircle = props => ( |
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 @stellarmagnet know we're using two different loading icons in the app?
…y, so the work can be picked up in the future
Everything is either fixed or asked about. To test and/or see changes please uncomment |
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.
A few more things commented inline. Also, this needs to be commented out: https://github.com/AutarkLabs/aragon-profile/blob/11-addMember/src/client/components/Profile.js#L23
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.
Partial review, so feedback can be addressed while I continue the rest of my review.
@rkzel this looks good except this line still needs to be commented out:
The organization panel still appears on small screen sizes. Once that change is fixed you can go ahead and merge this |
In order to test it all together I needed both working state and modal changing appropriately. Fake isMember function is just few lines of code and what it does is simply:
setTimeout(() => resolve(Math.random() >= 0.5), 5000)
TImeout, so 'checking' step is actually visible in the modal.
Solved by this PR are issues:
#11 create addMember functionality,
#12 Integrate add member functionality into the modal,