Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix clock line break in voice message player on monospace font #8815

Closed
wants to merge 2 commits into from
Closed

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jun 11, 2022

Fixes element-hq/element-web#22541

This PR applies the same fix as #8320 to the clocks on voice message player on the timeline and the message composer.

Before After
before1 after1
before2 after2
before3 after3

Purple: padding
Yellow: margin

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems labels Jun 11, 2022
@luixxiul luixxiul marked this pull request as ready for review June 11, 2022 15:38
@luixxiul luixxiul requested a review from a team as a code owner June 11, 2022 15:38
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

The changes overall are almost certainly sane, however this bit of the app is heavily designed and needs to meet those designs.

Ideally we'd have screenshot tests to prove that nothing has changed here, as there appears to be quite a lot which has changed by accident.

position: relative;
display: inline-block;
display: flex;
Copy link
Member

Choose a reason for hiding this comment

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

Nested flexboxes end up going poorly on Safari/Firefox, and I don't think we need it here.

// The waveform (right) has a 1px padding on it that we want to account for, otherwise
// inherit from mx_MediaBody
padding-right: 11px;
@mixin AudioPlayer;
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should be using a class rather than a mixin.

}

.mx_Clock {
min-width: $font-42px; // for flexbox
Copy link
Member

Choose a reason for hiding this comment

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

which flexbox?

}

// For audio player and voice message player
.mx_SeekBar,
Copy link
Member

Choose a reason for hiding this comment

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

should this be tightly scoped?

@@ -69,7 +59,6 @@ limitations under the License.
position: absolute;
left: 0;
height: 30px;
top: -2px; // visually vertically centered
Copy link
Member

Choose a reason for hiding this comment

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

this was moving the seek bar 2px up, which is still needed.

// For timeline-rendered playback, the clock is on the other side of the waveform.
& + .mx_Clock {
text-align: right;
.mx_MessageComposer_controls {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be targeting the composer here - we need to be targeting the PlaybackContainer classes itself.


// For timeline-rendered playback, the clock is on the other side of the waveform.
& + .mx_Clock {
text-align: right;
Copy link
Member

Choose a reason for hiding this comment

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

This text-align was fixing a layout issue that is introduced by this PR where the clock ends up not matching the designs:
image

(this PR)

There should be no padding or margin on the right side of the clock.

@luixxiul luixxiul marked this pull request as draft June 15, 2022 18:51
@luixxiul
Copy link
Contributor Author

The PR was initially created before the latest change was added to the player, and it looks like many of the changes of this PR got obsolete since then.

this bit of the app is heavily designed and needs to meet those designs.

I am not sure if I follow; where are those designs?

@turt2live
Copy link
Member

I don't think the designs are public, unfortunately :(

We have very strict tolerances on the padding/margins for the voice message components though - I'm going to look into adding screenshot tests for this area.

@luixxiul
Copy link
Contributor Author

If the design is not publicly available, there is not really something I could do here.. Closing for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clock line break in voice message player on monospace font
2 participants