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

Update reactions to new design #2751

Merged
merged 23 commits into from
Nov 15, 2024
Merged

Update reactions to new design #2751

merged 23 commits into from
Nov 15, 2024

Conversation

Half-Shot
Copy link
Member

@Half-Shot Half-Shot commented Nov 11, 2024

@robintown robintown changed the title Update redactions to new design Update reactions to new design Nov 11, 2024
@toger5
Copy link
Contributor

toger5 commented Nov 11, 2024

What is the reason for the yaxis asymmetry here:

.reactionPopupMenuModal > div > div {
  padding-inline: var(--cpd-space-6x) !important;
padding-block: var(--cpd-space-6x) var(--cpd-space-8x) !important;

To me it just looks off that the icons are not y centered

Comment on lines 97 to 110
@media (max-width: 660px) {
.footer {
grid-template-areas: ". buttons buttons buttons .";
}

.logo {
display: none;
}

.layout {
display: none !important;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Removing this will regress #2754

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like a case of Ctrl-X rather than Ctrl-C

@toger5
Copy link
Contributor

toger5 commented Nov 12, 2024

Is the very small width addressed in the PR?
Screenshot from 2024-11-12 18-12-40

@Half-Shot
Copy link
Member Author

Is the very small width addressed in the PR? Screenshot from 2024-11-12 18-12-40

Should help, although

Is the very small width addressed in the PR? Screenshot from 2024-11-12 18-12-40

yep!

@toger5
Copy link
Contributor

toger5 commented Nov 14, 2024

This has not yet been addressed: #2751 (comment)
Is there a reason for the asymmetry.

Maybe it would be interesting to explore adding some react spring (or maybe a css transition is enough) to open and close the expanded/non expanded panel?

@Half-Shot
Copy link
Member Author

This has not yet been addressed: #2751 (comment) Is there a reason for the asymmetry.

Maybe it would be interesting to explore adding some react spring (or maybe a css transition is enough) to open and close the expanded/non expanded panel?

That chunk didn't seem to be making any visual difference so removed it. I'm afraid my eyes might be failing me, but it looks y centered to me.

@toger5
Copy link
Contributor

toger5 commented Nov 14, 2024

Maybe you are looking at a different y-centering?

image

The two boxes have the same size (this is the screenshot from your examples)

@toger5
Copy link
Contributor

toger5 commented Nov 14, 2024

That chunk didn't seem to be making any visual difference so removed it

I think we still need to reduce it to:

padding: var(--cpd-space-6x);

otherwise we get the dialog default of -x10 which looks to generous

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

This looks nice.
What do you think about the idea of making the open/close of the reactions animated?
I also think the disabled state is a bit unfortunate (with the grey bg)
It would be nice if the icon would be a bit transparent...
But It seems we are just passing disabled to compound so this probably should not be done here.

@toger5
Copy link
Contributor

toger5 commented Nov 15, 2024

@Half-Shot I also think we can do a shortcut:

  • if the hand is raised and you press the raise hand button we can just lower the hand?
  • this looses the option to: "send reaction while raising hand" but i think that is worth the nicer ux of lowering immediately.

@Half-Shot
Copy link
Member Author

This looks nice. What do you think about the idea of making the open/close of the reactions animated? I also think the disabled state is a bit unfortunate (with the grey bg) It would be nice if the icon would be a bit transparent... But It seems we are just passing disabled to compound so this probably should not be done here.

I had a look at animating it, but it is a bit of a faff with the dialog present. I'll do a draft PR with some animations to follow this up.

The disabled state is a bit unfortunate, yeah. I think it's a compound problem yep.

@Half-Shot Half-Shot merged commit eed1b98 into livekit Nov 15, 2024
7 checks passed
@toger5
Copy link
Contributor

toger5 commented Nov 15, 2024

I had a look at animating it, but it is a bit of a faff with the dialog present. I'll do a draft PR with some animations to follow this up.

it its not as easy as addin a css tanstition duration we should not bother too much about it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants