-
Notifications
You must be signed in to change notification settings - Fork 48
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
Map over list of eventComment #3689
base: master
Are you sure you want to change the base?
Conversation
d46e1a4
to
9360a2b
Compare
haha sorry, but you'll have to change up the logic here with the migration over to |
)} | ||
{eventCommentFields.map((event, i) => { | ||
return ( | ||
event.active && ( |
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.
It makes more sense to filter the list instead. Either here or in the declaration.
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 code snippet is now updated. Could you check and tell me if filtering is needed now or not.
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 could try to put showComment
and the spyValue
part in the eventCommentFields
list, but maybe filtering isn't necessary now??
9360a2b
to
ac06d17
Compare
ac06d17
to
178f939
Compare
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.
LGTM:) lots of love for the reduction of duplicate code
{eventCommentFields.map((event) => { | ||
return spyValues((values: CompanyInterestFormEntity) => { | ||
const showComment = values.events?.some( | ||
(e) => e.name === event.name && e.checked === true | ||
); |
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.
Haven't looked into how these values work exactly, so I'm just throwing it out there so you can decide if it makes sense.
As you're checking the array for events that are .checked (I assume that you have covered all the event names in the array above - so that the e.name === event.name
check would only make sure that specific event is checked), could it be an option to instead do something like the following?
spyValues((values: CompanyInterestFormEntity) =>
values.events?.filter(event => event.checked).map(event => {
const eventFields = eventCommentFields[event.name]
return (...);
})
)
For it to make sense, eventCommentFields
would need to be an object with what is currently the name: ""
value as the key.
Just an alternative approach to do the same thing, but it would shorten it down a little bit more, and remove the need for the some()
calls and boolean values making it a little bit more readable(:
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.
+1 on @norbye comment, then
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.
Huge improvement! I like @norbye's suggestion, but I think it's fine as it is too:)
{eventCommentFields.map((event) => { | ||
return spyValues((values: CompanyInterestFormEntity) => { | ||
const showComment = values.events?.some( | ||
(e) => e.name === event.name && e.checked === true | ||
); |
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.
➕
@Arashfa0301 Any updates on this? Would be nice to eventually get it in |
@Arashfa0301 can we get this merged? |
Description
Improved companyInterestPage by mapping over a list of eventComment field instead of directly having the code snippets for each comment
Testing