-
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 team page member display #591
Conversation
[diff-counting] Significant lines: 574. |
70adb73
to
95c3819
Compare
Thanks for all the hard work on this Chris! Looks awesome so far. |
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 looks really good 🙌 🙌!! I'm wondering if we should display Technical PM as Technical Product Manager ?
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 this is exciting it looks nice :D
Left some comments, some are very trivial feel free to ignore those.
(Also enable format on save I promise it's a life changing decision if you haven't already)
import FA23Members from '../../../backend/src/members-archive/fa23.json'; | ||
|
||
const MemberDisplay: React.FC = () => { | ||
const [selectedRole, setSelectedRole] = useState<string>('Full Team'); | ||
const [selectedMember, setSelectedMember] = useState<IdolMember | undefined>(undefined); | ||
|
||
const memberDetailsRef = useRef<HTMLInputElement>(null); | ||
|
||
const allMembers = FA23Members.members as IdolMember[]; | ||
const roles: { | ||
[key in Role]: { | ||
roleName: string; | ||
description: string; | ||
members: IdolMember[]; | ||
order: string[]; |
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.
Ok so kinda tricky but - The original dti website used a different type definition for members NovaMember
. This is also in common-types
btw. The pull-from-idol
job used to have to convert between IdolMember
in the db and NovaMember
for the website. This in part was because they were somewhat developed separately (referring to idol and the old website.
In one sense I can see that unifying the type definitions might be useful to bypass the need for that conversion. On the other hand, it might be useful to still use separate types between the 2 different use cases since some IdolMember
data could potentially be only for internal purposes and doesn't need to be available on general website.
I'm thinking specifically w.r.t. to dev portfolios since all devs are required to have a github handle set for that to work but they don't necessarily want to have that displayed on their profile on the website. Maintaining a separate internal list of github handles is something we had considered as future work at the time of developing dev portfolios on idol but never actually got around to it.
Just something to consider. There is a json with NovaMember
type members at dti-website/src/data/all-members.json
tho if you do go in that direction.
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.
Yeah, dividing the two member types would be good for privacy reasons. Members can then edit what they want to display on their profile via IDOL, but that's another feature we can talk about.
The team page has risen some concerns about the current database, which we should resolve first before deciding on using NovaMember
(which I'm leaning towards). First, we need to keep track of the colleges of each member. Majors should be standardized (CS vs. Computer Science) -- these are important when determining the represented colleges and majors in the About portion of the page. Another issue is how to deal with multiple subroles within a main role, akin to the advisor discussion on a different PR. That is, how to expand RoleDescription
or some other field to include advisors, APM, PMM, different lead roles, etc.
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.
w.r.t. NovaMember
vs IdolMember
, that's really just about whether to use the same data and fields bewteen idol and the dti website. The NovaMember
type can of course be changed to fit whatever the needs of the new dti website are.
w.r.t. profile stuff:
- The subroles situation i kinda touched on in Add Advisor Role #588. Related to Permissions revamp #471 as well. I think that's the best way to handle this.
- In terms of standardizing majors, there's a tradeoff that needs to be considered there. The IDOL profile editor is intended to allow self-servicing of changing things like your major. That's just a free-form text box to type your major in. The only "verification" happens at the stage when an idol admin has to check the diff before approving it. I'm not sure at that stage if rejecting a change is worth it because someone put "CS" instead of "Computer Science." The way to prevent that might be to use a dropdown with all the majors, but that requires maintaining a list of majors which is a pain. From a design perspective, I'm not sure where they stand on that, but from an engineering perspective, it might be fine imo as long as it's not something egregious like the major is misspelled or it's a fake major or something.
<p className="mt-6 text-lg"> | ||
Learn more about the team at DTI and what we do behind the scenes. Our design, | ||
development, business, and product teams all strive to use creativity and innovation to | ||
make DTI's products more impactful and functional. |
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.
make DTI's products more impactful and functional. | |
make DTI's products more impactful and useable. |
I'm not sure what the design says but more functional feels like it implies that it wasn't very functional before haha. Feel free to ignore 😆
Commit Changes
|
Just for informative purposes, if you split up the single refactoring and fixing nits commit into 3 commits with these exact commit messages, you wouldn't need this comment :D. That's kinda the purpose of using commits and having non-trivial commit messages in the first place. You can even selectively stage part of a file to be committed using And besides that, it can also be helpful when reviewing because the reviewer can go commit by commit through fairly isolated changes. |
6d07170
to
2ddbb7d
Compare
new-dti-website/src/app/utils.ts
Outdated
}, | ||
allMembers: IdolMember[] | ||
) => { | ||
for (const [key, value] of Object.entries(roles)) { |
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.
Instead of the loop, try to do this more functionally with return Object.entries(roles).map(...)
instead of editing this in place, which makes it harder to follow (i.e. if this breaks, we'll have a harder time fixing it). Slack me if you need help!
This might involve changing more than just this function, but the refactor will be worth it for code readability.
Also I'm taking this review over from @henry-li-06 (spoke with him already), so no need to wait for his approval. |
* make populateMembers functional * add consts.ts * comment canInsertMemberDetails
FYI the deploy preview: https://deploy-preview-591--new-cornell-dti.netlify.app/ is crashing now. |
* make populateMembers functional * add consts.ts * comment canInsertMemberDetails
Summary
This pull request is the first step towards implementing the team page
Notion/Figma Link
https://www.figma.com/file/jKbvR1jVr1k80GGWnCpiRT/Website-Designs-SP23%2FFA23?type=design&node-id=5464-9095&mode=design&t=5jZUX1RMV8eKUdRC-0
https://www.notion.so/Team-Page-Team-Member-Display-5d8845dc798f4145b541c4282c4a1417
Test Plan
see video!
https://github.com/cornell-dti/idol/assets/144409850/a4fe1d6a-0f0b-4dc1-b1cd-9fb3f81d7309
Notes