Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature: Officer Section #21
Feature: Officer Section #21
Changes from 12 commits
c9ace03
b63b989
a871c1a
9995f16
7b0529b
82488d2
f24d6fe
a23f6cf
60bafa6
04288a4
49c7921
1643417
244647c
850045f
cc1800a
9cdf600
91ae909
e9372c3
9b28c0b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
[Comment]: Good callout & use of comments. May be worth looking into later, but let's accept this limitation for now. I'll sync with you later and see if we should open a backlog ticket to dig into this 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.
[Comment/Suggestion]: It's unusual to see an
alt
text property for a generic component like this (especially given it's set to an empty string when used below). I would suggest computing something likeProfile picture of ${name}.
instead, I think that's good enough for our needs.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.
[Comment]: This may be a good candidate to have in a separate file, that way it can just be referenced by this page. I'd expect this constant to get a lot messier once we start adding previous officers.
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 agree. I like to put data like this in separate files. In previous projects, I've stored data in
.ts
files in a separatedata/
directory. We could do that too, or put data in thesrc/app/about/
directory. I'd like to know what you think (and if this should maybe be saved for a separate issue).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'm generally inclined to always abstract by usage - in other words, a simple
officers.ts
file (orofficer-data.ts
, if preferred) in theabout
directory is probably the best option for now. I'm not particularly a fan of diving projects up into categories like/components
,/forms
,/types
, etc. I think it forces more abstraction than is often needed (or conceptually present) and grouping by programmatic concern is less valuable than grouping by business concern.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.
[Suggestion]: You can use
{...cardData}
to spread props into the component, save a bit of work/duplication here.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.
Can't change this due to eslint react/jsx-props-no-spreading. I've decided to keep it as is rather than disable the rule. Let me know if you insist this be changed.
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.
That's fine, no strong opinion here. I'd guess that the use of TypeScript also helps catch many of the common bugs here.
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.
[Comment]: Will probably have to abstract this down the line when we add past officers. Don't change anything here; just making note and musing for the future.