Page MenuHomePhabricator

Spaces instead of css margin/padding in new Related changes
Open, Needs TriagePublic

Description

It seems a little strange to use spaces to align the lists in 2018.


Event Timeline

Iniquity created this task.May 11 2018, 7:39 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 11 2018, 7:39 AM
Vvjjkkii renamed this task from Spaces instead of css margin/padding in new Related changes to u5caaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from u5caaaaaaa to Spaces instead of css margin/padding in new Related changes.Jul 1 2018, 4:18 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
Restricted Application added a project: Growth-Team. · View Herald TranscriptApr 1 2019, 5:42 PM
Jdlrobson added a subscriber: Jdlrobson.

FWIW removing these breaking spaces would help us get RecentChanges on mobile much quicker.

Not having full context, on why the spaces landed there. Hopefully @Mooeypoo or @Catrope could shed a light…?

This is the quite entangled view of the "Enhanced" recent changes table. The structure is tabular, but this specific cell has several notations in it (like "N" for new page, etc) and in order to align them across the different entries, the line is either with the symbol, or with a space.

Basically, it's doing something like this:

_____00:00
_N___00:00
__*__00:00
_N*__00:00

Etc... using fixed-width font to make the grouping of characters always appear or not appear where they're supposed to be and still have that line "aligned".

This is coming from ChangesList.php#recentChangesFlags:

	public function recentChangesFlags( $flags, $nothing = "\u{00A0}" ) {
		$f = '';
		foreach ( array_keys( $this->getConfig()->get( 'RecentChangesFlags' ) ) as $flag ) {
			$f .= isset( $flags[$flag] ) && $flags[$flag]
				? self::flag( $flag, $this->getContext() )
				: $nothing;
		}

		return $f;
	}

Note that \u{00A0} is a non-breaking space.

This should definitely be fixed, but the work on this is a bit more elaborate than it should be. EnhancedChangesList.php is a pretty messy output in general that tries to utilize new AND old outputs to try and group things in table cells and yet mixes the above fixed-font behavior that you reported on. On top of that, as you can see above, this behavior is shared not just with EnhancedChangesList but also with RecentChanges and RelatedChanges without the group-by-page feature. It's relatively deep in that code, unfortunately.

We tried to make it a bit cleaner for RCFilters, but it really needs a pretty heavy rewrite.

What's the proposed alternative? A table where each cell contains either nothing or one character?

FWIW removing these breaking spaces would help us get RecentChanges on mobile much quicker.

Why would it make things easier for you? What about the nbsp hack is causing problems for mobile?

@Catrope in mobile we're keen to stack a bunch of these and have more granular control of the positioning. The table layout itself is problematic, so that might need to go long term, but on the short term any invisible character creates problems as it cannot be styled so introduces spacing where it's not wanted with no way to remove it. It sounds like the original intent could be swapped out for CSS or HTML inline block elements with a fixed width.

My expectation is that either growth team or us will work on it and web will only do that if and when we decide to work on incorporating the recent changes feed. I'm putting this in tracking and AMC to account for the latter case.

kostajh moved this task from Inbox to Triaged but Future on the Growth-Team board.Jul 18 2019, 1:52 AM
kostajh added a project: patch-welcome.
kostajh added a subscriber: kostajh.

My expectation is that either growth team or us will work on it and web will only do that if and when we decide to work on incorporating the recent changes feed. I'm putting this in tracking and AMC to account for the latter case.

We're unlikely to work on this in the short-to-medium term but could look at a patch if someone else submits.