-
Notifications
You must be signed in to change notification settings - Fork 23
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
Unify officer cards. #333
Unify officer cards. #333
Conversation
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.
Since the card files were removed, did you also remove the now irrelevant SCSS styling from jedi.module.scss
and impact.module.scss
?
There's also quite a few places where we probably want a non breaking space instead of a normal one, so it may be best to replace them?
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.
Exciting stuff! I'll just comment with some overall things to consider:
- When possible, I would take advantage of the
children
prop; you can write a unified wrapper for mobile / column-wrapping behaviour (a "base" officer card of sorts), and then fill in the relevant information that differs based on the context as a child (ex "Class of...", position information, committee etc. This composition vs inheritance section in the React docs explains the "React way" of doing this! - If you want to simplify the props chain, you can create a schema for an "Officer" object that has some mandatory fields (name, pronouns, position, ...) and then pass that in. Optional props like committee or bio can then be passed in, but you can simplify usage a little bit!
- The styling for the compact leadership card is not very good; feel free to rework this however you want!
- For the tab whitespace changes, you may get a cleaner diff if you open a different PR with just the whitespace changes, rebase this PR off that branch, and then merge that first. Thanks for catching the tab inconsistency!
Done. |
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.
I'll leave it to Advaith to give a final review, but I personally think the changes look + work well! Solid work as always, Dylon!
(feel free to punt the feedback from my previous review somewhere else - I know it's quite a bit. I still do think that they may be good for long-term work, but also amenable to whatever the dev team wants)
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.
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.
Other than that, it looks good - so we should be good to merge once that's fixed! I'll take into account Matt's previous review as long-term things we should do or issues we can give interns / tla dev team!
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.
Feel free to re-request review once you're done with the changes!
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.
looks good! thanks for working on this Dylon!
@matthewcn56 make sure you take a look at this pr and merge main into your branch before updating leadership! |
alright will take a looksie! |
Addresses #264