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.

Sign up for GitHub

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

Jump to bottom

Add reactions to meetings. Rewrote meeting from class component -> hook. #3548

Merged
merged 1 commit into from Mar 16, 2023
Merged

Add reactions to meetings. Rewrote meeting from class component -> hook. #3548

merged 1 commit into from Mar 16, 2023

Conversation

jonasdeluna
Copy link
Member

image

@jonasdeluna jonasdeluna mentioned this pull request Feb 10, 2023
Merged
@github-actions github-actions bot added the review-needed Pull requests that need review label Feb 10, 2023
@ivarnakken
Copy link
Member

Cool! 💯 We can use this to register who wants pizza for our meetings haha!

Thought; could we perhaps move it down to the comment section? I feel like the sidebar is only for general info + admin stuff.

@jonasdeluna
Copy link
Member Author

jonasdeluna commented Feb 10, 2023 •
edited

#3548 (comment)
Can try this 👍

@jonasdeluna
Copy link
Member Author

jonasdeluna commented Feb 10, 2023 •
edited

image
Like this?

It does sort of make sense for it to be in the original position (ref #3548 (comment)) to make it clear it's part of the meeting response, and not a reaction on the meeting itself?

ivarnakken
ivarnakken approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member

@ivarnakken ivarnakken left a comment •
edited

Choose a reason for hiding this comment

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

Awesome! 🍕

Also, small tip; your commit could have been split into two separate ones for easier code review.

app/routes/meetings/MeetingDetailRoute.tsx Outdated Show resolved Hide resolved
@ivarnakken ivarnakken added new-feature Pull requests that introduce a new feature approved Pull requests that have been approved chore Pull requests that does something "boring", yet important, e.g. cleaning up code and removed review-needed Pull requests that need review labels Feb 10, 2023
@github-actions github-actions bot added the review-needed Pull requests that need review label Feb 10, 2023
@jonasdeluna
Copy link
Member Author

^removed & placed emojis at the bottom

fetchingEmojis: state.emojis.fetching,
fetching: state.meetings.fetching,

LudvigHz
LudvigHz approved these changes Feb 10, 2023
View reviewed changes
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.

Cool, this is a super-nice feature and as mentioned could be useful for more than just fun :)/ Just some questions :)

I also liked having the reactions someplace more visible than under the content, as I feel they "disappear" if you have just a little text in the meeting

app/routes/meetings/MeetingDetailRoute.tsx Outdated
) => (loggedIn ? dispatch(fetchMeeting(meetingId)) : Promise.resolve());
) =>
loggedIn
? dispatch(fetchMeeting(meetingId), fetchEmojis())
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you can dispatch like this? According to the docs, this is not an option.
You could rather do a Promise.all([dispatch(fetchmeeting(...)), dispatch(fetchemojis())]

Copy link
Member

Choose a reason for hiding this comment

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

Also, why are you fetching emojis here and passing fetchEmojis into the reaction picker?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I'm not entirely sure actually. It was written that way in the ArticleDetailRoute.tsx file, but I'll try the promise.all instead. When not fetching he emojis and passing them the emojipicker component doesn't load properly (at all)

@ivarnakken ivarnakken added the changes-requested Pull requests with requested changes label Feb 10, 2023
@jonasdeluna
Copy link
Member Author

image

Changed placement back to the sidebar (?)

@ivarnakken
Copy link
Member

I also liked having the reactions someplace more visible than under the content, as I feel they "disappear" if you have just a little text in the meeting

@LudvigHz
The same goes for comments though. I feel like "interactions" like comments and reactions should be grouped together? Because that's pretty common. Besides, I feel like comments are more important than reactions, and we've put that at the bottom.

@LudvigHz
Copy link
Member

The same goes for comments though. I feel like "interactions" like comments and reactions should be grouped together? Because that's pretty common. Besides, I feel like comments are more important than reactions, and we've put that at the bottom.

Yeah, I agree. And it is a bit annoying sometimes that comments are all the way at the bottom, but on the other hand, comments don't really fit anywhere else, whereas reactions can fit in a small space almost anywhere, so it's more flexible where we can put it

@jonasdeluna
Copy link
Member Author

Could not reproduce the cypress fail, but may be due to lack of the correct backend version?

@jonasdeluna jonasdeluna force-pushed the pizzareaction branch 4 times, most recently from d0e93be to 7bab568 Compare March 7, 2023 20:06
LudvigHz
LudvigHz requested changes Mar 7, 2023
View reviewed changes
cypress/support/component.js Outdated Show resolved Hide resolved
@ivarnakken ivarnakken removed the review-needed Pull requests that need review label Mar 15, 2023
@github-actions github-actions bot added the review-needed Pull requests that need review label Mar 16, 2023
LudvigHz
LudvigHz reviewed Mar 16, 2023
View reviewed changes
app/routes/pages/components/subcomponents/Statistic/Statistic.css Outdated
@@ -9,7 +9,7 @@
}

.statistic {
font-size: 2.2rem;
font-size: 1.2rem;
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

app/routes/pages/components/LandingPage.css Outdated
@@ -56,7 +56,7 @@ div > .locationContainerItem {

.contactTitle {
color: var(--lego-font-color);
font-size: 1.7rem;
font-size: 1rem;
Copy link
Member

Choose a reason for hiding this comment

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

What does this do and why is it part of this PR?

@jonasdeluna jonasdeluna merged commit 9a53062 into master Mar 16, 2023
2 checks passed
@jonasdeluna jonasdeluna deleted the pizzareaction branch March 16, 2023 13:11
@jonasdeluna jonasdeluna mentioned this pull request Mar 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivarnakken ivarnakken ivarnakken approved these changes

@LudvigHz LudvigHz LudvigHz approved these changes

Assignees

@jonasdeluna jonasdeluna

Labels
approved Pull requests that have been approved changes-requested Pull requests with requested changes chore Pull requests that does something "boring", yet important, e.g. cleaning up code new-feature Pull requests that introduce a new feature review-needed Pull requests that need review
Projects
None yet
Milestone
No milestone
3 participants
@jonasdeluna @ivarnakken @LudvigHz