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

Fix cropped message reaction icons #5174

Merged
merged 1 commit into from Apr 14, 2021
Merged

Fix cropped message reaction icons #5174

merged 1 commit into from Apr 14, 2021

Conversation

jost-s
Copy link
Contributor

@jost-s jost-s commented Apr 12, 2021

Contributor checklist:

Description

Reduced the min-width/increased the max-width of media queries to switch sooner from reaction icons for messages to a "more" menu.

Smallest media query
image

next smallest media query
image

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

I'll test this soon, but left a question before I do so.

@@ -11476,15 +11476,15 @@ $contact-modal-padding: 18px;
}

/* Spec: container < 437px */
@media (min-width: 0px) and (max-width: 799px) {
@media (min-width: 0px) and (max-width: 834px) {
.module-message {
// Add 2px for 1px border
max-width: 302px;
}
}

/* Spec: container > 438px and container < 593px */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these comments need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps they do have to change. I don't understand what they're referring to. If that Spec: container < 437 px refers to the module-timeline__message-container, then they're not up to date.
Should I update them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Contributor Author

@jost-s jost-s Apr 13, 2021

Choose a reason for hiding this comment

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

I'm thinking it's this container as mentioned before
image

Before my commits the values in the comments didn't match that container's width. I've updated them now to its width anyway.

@EvanHahn-Signal
Copy link
Contributor

Just pulled down the changes which look good—all that's needed is updates to those comments and then we should be good to go.

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for doing this.

@EvanHahn-Signal EvanHahn-Signal merged commit 1493c1b into signalapp:development Apr 14, 2021
@jost-s jost-s deleted the bugfix/keep-message-menu-button-visible branch May 15, 2021 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants