status component: Fix repeater name overflowing #383

yukijoou · 2024-04-01T15:04:26Z

yukijoou commented

2024-04-01 15:04:26 +00:00

If someone repeating a post had a long username, their username would overflow beyond the bounds of the post.

This fixes this isse by turning the bar displaying the username and repeat icon into a flexbox.

If someone repeating a post had a long username, their username would overflow beyond the bounds of the post. This fixes this isse by turning the bar displaying the username and repeat icon into a flexbox.

Oneric reviewed 2024-05-17 17:03:46 +00:00

Oneric left a comment

Hi, thanks for the fix.

I’m far from an expert in CSS, but it seems to work fine in testing. Though i have some – potentially silly – questions. Could you explain those parts in more detail?

Hi, thanks for the fix. I’m far from an expert in CSS, but it seems to work fine in testing. Though i have some – potentially silly – questions. Could you explain those parts in more detail?

src/components/status/status.scss Outdated

					
				@ -87,7 +87,6 @@

				  .status-username {

				    white-space: nowrap;

				    overflow: hidden;

				    max-width: 85%;

Oneric commented

2024-05-17 16:39:22 +00:00

this removes max-width for usernames above all statuses regardless of whether it’s the author’s name or a repeater name, but only new restrictions for repeater names are added

Are you sure this doesn't introduce any new overflow issues? Which other rule already ensures this?

this removes `max-width` for usernames above _all_ statuses regardless of whether it’s the author’s name or a repeater name, but only new restrictions for repeater names are added Are you sure this doesn't introduce any new overflow issues? Which other rule already ensures this?

yukijoou commented

2024-05-17 23:24:29 +00:00

I'm honestly not sure why I removed this in the first place, I think this is a leftover from an earlier attempt at fixing the bug. Reverting this change doesn't seem to cause any issue, so I'll just do that to avoid introducing regressions!

yukijoou marked this conversation as resolved

src/components/status/status.vue Outdated

					
				@ -111,0 +110,4 @@

				          <div

				            class="repeat-text"

				          >

				            {{ $t('timeline.repeated') }}

Oneric commented

2024-05-17 16:42:44 +00:00

This only includes the actual “repeated” text into the flex-shrink: 0; section. Testing the change shows the icon doesn’t get hidden regardless. Do you think it’d make sense to still include the icon into the flex-shrink: 0; section anyway to better communicate the desired output (not hiding the icon)?

This only includes the actual “repeated” _text_ into the `flex-shrink: 0;` section. Testing the change shows the icon doesn’t get hidden regardless. Do you think it’d make sense to still include the icon into the `flex-shrink: 0;` section anyway to better communicate the desired output (not hiding the icon)?

yukijoou commented

2024-05-17 23:18:51 +00:00

indeed, seems like this would be cleaner!

yukijoou marked this conversation as resolved

yukijoou force-pushed fix-status-usernames from 578b82f871 to 8c49474dea

2024-05-17 23:25:57 +00:00

Compare

yukijoou commented

2024-05-17 23:26:27 +00:00

this should be all good now :) thanks for the review!

Oneric approved these changes 2024-05-18 00:37:52 +00:00

Oneric left a comment