Page MenuHomePhabricator

Minor issues with some of Minerva skin's table element's attribute definitions
Closed, ResolvedPublic

Description

I've come across at least one minor "typo" in the definitions found in skins.minerva.content.styles / tables.less and question a couple of others.

Rather than trying to squeeze them all into this section, I'm going to give each issue its own post and number them accordingly for any further discussion purposes.

Event Timeline

GOIII created this task.Jul 14 2015, 12:07 AM
GOIII raised the priority of this task from to Needs Triage.
GOIII updated the task description. (Show Details)
GOIII added subscribers: GOIII, Jdlrobson, Krenair.
Restricted Application added a project: Readers-Web-Backlog. · View Herald TranscriptJul 14 2015, 12:07 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
GOIII added a comment.EditedJul 14 2015, 12:10 AM

1.— For the sake of correctness if nothing else, there is redundancy in the table-cell defs under the .wikitable table-class when in 'mobile mode' (this is the 'typo' one).

In the current rule...

.content table.wikitable > tr > th,
.content table.wikitable > tr > th,
.content table.wikitable > * > tr > th,
.content table.wikitable > * > tr > td {
	border: 1px solid #EEE;
	padding: .2em;
}

... the first and second attributes defined are identical (Lines 29 & 30). Thankfully, the last defined attribute compensates for this redundancy. The "correct" rule should be

.content table.wikitable > tr > th,
.content table.wikitable > tr > td,
.content table.wikitable > * > tr > th,
.content table.wikitable > * > tr > td {
	border: 1px solid #EEE;
	padding: .2em;
}
GOIII added a comment.EditedJul 14 2015, 12:11 AM

2.— When not under the min-width @ rule (e.g. less than min-width 768px), the display attribute is set to block rather than table for the TABLE element. It seems to be set to table in those Hi-res cases per the rule though.

Specifically the current attribute in question is defined as (Line 14)...

.content table
	display: block;
}

I can't see any reason why the TABLE element's display: attribute should be set to block instead of table regardless of the screen width in effect. Could there be some Mobile-specific reasoning behind this and, if so, please elaborate?

GOIII added a comment.EditedJul 14 2015, 12:11 AM

3.— When not under the min-width @ rule (e.g. less than min-width 768px), the display attribute is set to block rather than table-caption for the caption element. I can't find anything that shows this set any different in Hi-res cases per the rule either (but I've been wrong before).

Specifically the current selector attributes in question are defined as (Lines 17 to 19)...

.content table caption
	display: block;
	text align: left;
}

I can't see any reason why this should be block instead of table-caption too. Plus the normal text-alignment for table captions is center not left. Any logic to forcing deviations away from the expected norm?

TheDJ added a subscriber: TheDJ.Jul 15 2015, 9:11 AM

I can't see any reason why the TABLE element's display: attribute should be set to block instead of table regardless of the screen width in effect. Could there be some Mobile-specific reasoning behind this and, if so, please elaborate?

There is. Table display form is really unfriendly to narrow view. It causes your viewport to overflow real easily and it causes a ton of 'edge' case exceptions when it interacts with adjacent or parent elements. It's much easier to have it as block really.

I can't see any reason why this should be block instead of table-caption too. Plus the normal text-alignment for table captions is center not left. Any logic to forcing deviations away from the expected norm?

The reason to use block, i'm not entirely sure of here, but it probably has something to do with adapting behavior of the caption to match behavior of the table being set to display.
As regards to alignment, I'm assuming it is because centered content on a narrow screen gives a very cluttered look, especially if you cannot make predictions about what kind of content you will have to show.

Btw. best not use the term hi-res to describe wide screens. Hi-res is often associated with high dpi screens, like Retina, so it's a bit confusing.

GOIII added a comment.Jul 15 2015, 3:21 PM

. . .
Btw. best not use the term hi-res to describe wide screens. Hi-res is often associated with high dpi screens, like Retina, so it's a bit confusing.

Gotcha. Edited. and Thanks for the explanations.

I'm a little confused by this task. Could the issues please be given a high level summary in the description? Having them in comments is confusing. Thank you! It seems the only issue is .content table.wikitable > tr > th is duplicated or have I missed something?

Change 225031 had a related patch set uploaded (by TheDJ):
Correct typo in tables stylesheet of minerva

https://gerrit.wikimedia.org/r/225031

Jdlrobson moved this task from Backlog to Bugs on the MobileFrontend board.Jul 20 2015, 5:15 PM

Change 225031 merged by jenkins-bot:
Correct typo in tables stylesheet of minerva

https://gerrit.wikimedia.org/r/225031

Jdlrobson closed this task as Resolved.Jul 20 2015, 6:50 PM
Jdlrobson claimed this task.