Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type and rewrite components as function-components #3795

Merged
merged 5 commits into from Apr 17, 2023

Conversation

eikhr
Copy link
Member

@eikhr eikhr commented Apr 16, 2023

Description

  1. Rewrote Comment-component to a function component and typed it's props plus some props in other related components.
  2. Did the same thing to the Quote and QuoteList components.
  3. Rewrote Reaction- and Reactions-components as functions, and typed a bunch of reaction related stuff
  4. Removed unused reaction entity-reducer from redux, and improved state type for mutateReactions mutator.
  5. Type and clean up meeting components. Mostly improved types and made MeetingAnswer a function-component. Spent some time looking at the MeetingAnswer-component that had a few bugs.

For reviewing I highly recommend selecting "No whitespace" as refactoring like this often changes the indentation level, leading to huge amounts of whitespace changes.

Result

We are now under 1000 ESLint warnings! 馃コ
Screenshot 2023-04-16 at 22 21 20

And here is the MeetingAnswer-screen, which now works (it only shows up when not logged in)
Before:
image

After: (it now shows if you accepted or declined, and the login-button works)
Screenshot 2023-04-16 at 22 24 13

Poem

馃 Generated by Copilot at 3f5c1ae

Sing, O Muse, of the valiant code review
That brought new types and functions to the app
And made the code more readable and true
With ID, Emoji, and ContentTarget as the map

Testing

  • I have thoroughly tested my changes.

I've been clicking around the pages, making sure I didn't make any functional changes.


Resolves webkom/lego#2891

@eikhr eikhr added types Pull requests that improve or fix types review-needed Pull requests that need review technical-debt Pull requests that reduces technical debt chore Pull requests that does something "boring", yet important, e.g. cleaning up code labels Apr 16, 2023
@eikhr eikhr changed the title Rewrite Comment component to functional Type and rewrite components as function-components Apr 16, 2023
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very sexy PR - I love to review this kind of stuff 馃挴

app/utils/getEntityType.ts Show resolved Hide resolved
Comment on lines +28 to +31
export type EmojiWithReactionData = Emoji & {
hasReacted: boolean;
reactionId: ID;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps move this to a dedicated models file, since it is used other places?

app/routes/quotes/components/QuoteList.tsx Outdated Show resolved Hide resolved
app/routes/meetings/components/MeetingAnswer.tsx Outdated Show resolved Hide resolved
app/reducers/quotes.ts Outdated Show resolved Hide resolved
@eikhr eikhr force-pushed the functional-components branch 2 times, most recently from c0b501c to 0e27d20 Compare April 16, 2023 22:53
Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 馃挴 馃

Comment on lines +68 to +69
e.target instanceof Node &&
nodeRef.current.contains(e.target)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeRef is a ref to the current emoji-picker div-element
e.target is the thing that was clicked

If we click inside the emoji-picker, nodeRef.current.contains(e.target) === true, and we don't want to close it. All other clicks should close it.

app/components/UserAttendance/withModal.tsx Show resolved Hide resolved
app/reducers/meetingsToken.ts Show resolved Hide resolved
app/reducers/meetingsToken.ts Show resolved Hide resolved
app/routes/meetings/MeetingDetailRoute.tsx Outdated Show resolved Hide resolved
@LudvigHz LudvigHz added review-needed Pull requests that need review approved Pull requests that have been approved and removed review-needed Pull requests that need review labels Apr 17, 2023
@LudvigHz LudvigHz merged commit b3da08d into master Apr 17, 2023
4 checks passed
@LudvigHz LudvigHz deleted the functional-components branch April 17, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved chore Pull requests that does something "boring", yet important, e.g. cleaning up code review-needed Pull requests that need review technical-debt Pull requests that reduces technical debt types Pull requests that improve or fix types
Projects
None yet
3 participants