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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a button that show who has reacted on pizza and not #5109

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ShaileshS1702
Copy link
Contributor

@ShaileshS1702 ShaileshS1702 commented Oct 26, 2024

Description

This commit makes a new pizza button component that when hovered display who has wants pizza and who does not. It also makes the reaction cleaner, by sorting alphabetically and newline for each person.

As of now the pizza button is a button, but does not do anything. There is plan to connect it to the @nye(2023)'s project pizza on a later date. Any tips here are welcome.
Also as of now the button is only visible if at least one person has reacted with pizza to the meeting. There is a plan to make "pizza mode" and option when making a new meeting/updating a meeting rather than just who has reacted.

Result

If you've made visual changes, please check the boxes below and include images showing the changes. Descriptions are appreciated.

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.

Caution

Make sure your images do not contain any real user information.

Description Before After
Made reaction have newline for each user and sort it (which is not visible due to privacy) image image
The pizza button on dark wide screen New feature image
No pizza button when there are no pizza reactions new feature image
Show pizza button when hovered new feature image
Light mode of pizza button hovered New feature image
Mobile view New feature image

Testing

  • I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.
Tried different meetings with different amount of people and tried light mode and screen width. Only issue I can think of is if someone has a very long name. I was not able to test would happen then.

Resolves ABA-1132

@ShaileshS1702 ShaileshS1702 added the new-feature Pull requests that introduce a new feature label Oct 26, 2024
Copy link

linear bot commented Oct 26, 2024

Copy link

vercel bot commented Oct 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
lego-bricks-storybook ⬜️ Ignored (Inspect) Visit Preview Oct 30, 2024 8:28pm

@github-actions github-actions bot added the review-needed Pull requests that need review label Oct 26, 2024
@ShaileshS1702 ShaileshS1702 self-assigned this Oct 26, 2024
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 cool and useful feature!

app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
app/routes/meetings/components/MeetingDetail.tsx Outdated Show resolved Hide resolved
app/routes/meetings/components/MeetingDetail.tsx Outdated Show resolved Hide resolved
app/components/ContententList/index.tsx Outdated Show resolved Hide resolved
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
@ivarnakken ivarnakken added the changes-requested Pull requests with requested changes label Oct 26, 2024
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
app/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
app/components/PizzaMeetingButton/index.tsx Outdated Show resolved Hide resolved
@ivarnakken
Copy link
Member

When I come to think of it - could we maybe support this directly on the pizza reaction button? Instead of having it on a separate button
image

app/routes/meetings/components/PizzaButton.tsx Outdated Show resolved Hide resolved
app/routes/meetings/components/PizzaButton.tsx Outdated Show resolved Hide resolved
app/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
app/routes/meetings/components/PizzaButton.tsx Outdated Show resolved Hide resolved
@ShaileshS1702
Copy link
Contributor Author

When I come to think of it - could we maybe support this directly on the pizza reaction button? Instead of having it on a separate button
image

We could have it integrated onto the pizza reaction button. I have of sometime in the future (it may take a while) to connect it to the pizza project @nye(2023) had. Is it better to integrate it into the pizza button now and maybe make it its own thing later?

@ShaileshS1702
Copy link
Contributor Author

Is there a general rule on which imports should come first? The linter tells me to move imports a lot, it would be easier to just do it right the first time

@ivarnakken
Copy link
Member

Is there a general rule on which imports should come first? The linter tells me to move imports a lot, it would be easier to just do it right the first time

It's a linting rule defined here. If you use vscode you can enable auto sorting on save: https://dev.to/shanesc/how-to-sort-and-cleanup-imports-on-save-in-vs-code-52p1

@ShaileshS1702
Copy link
Contributor Author

Is there a general rule on which imports should come first? The linter tells me to move imports a lot, it would be easier to just do it right the first time

It's a linting rule defined here. If you use vscode you can enable auto sorting on save: https://dev.to/shanesc/how-to-sort-and-cleanup-imports-on-save-in-vs-code-52p1

Thank you, that is so useful!!

Button is only visible when pizza is reacted with

Make a component that makes a list of items from string with newline

Make a pizza button

Make a pizza button that displays when atleast one person has reacted with 🍕. The button displays who has reacted on pizza and who has not.

Make reactions use ContentList for nicer formatting

fix linting
@jonasdeluna
Copy link
Member

This is very cool!

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.

Looking good! 😍 Just a slight question about your changes to the Reaction component

Comment on lines +99 to +105
.map((name) => <div key={name}>{name}</div>);
const tooltipContent = tooltipContentNames ? (
<div>
{tooltipContentNames}
har reagert med {emoji}
</div>
) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Why the added divs etc? Did the current way not work?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's to get them on separate lines. Now all the names are all on one long line, and it's impossible to read them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I made a component to take care of the information in tooltips. The component became redundant. Now the main reason the divs are there , as Eik said, are to make it cleaner and separate each name on a new line. There may be better ways to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Can you use a shard (<></>) with display: block, or just insert newline characters (\n)?

Copy link
Member

Choose a reason for hiding this comment

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

showPeople && optimisticUsers?.map((user) => user.fullName).join(', \n');
Something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shards may work ( I am not sure if shards are inline or blocks). My original plan was, as you said, to use \n, but it gets interpreted as HTML or something and and newlines just get switched out with spaces (I may have done it wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out the shards are also inline

Copy link
Member

Choose a reason for hiding this comment

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

Shards are not rendered as html, so it will work exactly the same as just having the names directly after each other

Copy link
Member

Choose a reason for hiding this comment

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

But adding "\n" after each line seems like a good solution imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My original plan was to use \n, but it gets interpreted as HTML or something and newlines just get switched out with spaces (I may have done it wrong). How should I use implement newlines?

@ShaileshS1702
Copy link
Contributor Author

Good to merge?

Comment on lines +47 to +53
return (
<Tooltip content={tooltipContent}>
<Button>
<Pizza /> Pizza-reaksjoner
</Button>
</Tooltip>
);
Copy link
Member

Choose a reason for hiding this comment

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

It would be really nice if you could press this to react/unreact with pizza emoji!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a plan to make the button redirect to @nye(2023)s pizza project on a later date.
I can make the button temporarily react/unreact with the pizza emoji, but feel like having a button changing its purpose later might make it confusing. (It might be smart in case I do not actually ever finish the pizza project). What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes-requested Pull requests with requested changes new-feature Pull requests that introduce a new feature review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants