r67784 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r67783‎ | r67784 | r67785 >
Date:08:01, 10 June 2010
Author:kaldari
Status:ok (Comments)
Tags:
Comment:
putting local sitenotices in their own div and fixing sitenotice styling issues with Vector
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/skins/vector/main-ltr.css (modified) (history)
  • /trunk/phase3/skins/vector/main-rtl.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/vector/main-ltr.css
@@ -929,12 +929,12 @@
930930 font-size: 0.8em;
931931 margin: 0;
932932 }
933 - #siteNotice div,
934 - #siteNotice p {
935 - margin: 0;
936 - padding: 0;
937 - margin-bottom: 0.9em;
938 - }
 933+#centralNotice {
 934+ margin-bottom: 0.9em;
 935+}
 936+#localNotice {
 937+ margin-bottom: 0.9em;
 938+}
939939 /* Categories */
940940 .catlinks {
941941 border: 1px solid #aaa;
Index: trunk/phase3/skins/vector/main-rtl.css
@@ -929,12 +929,12 @@
930930 font-size: 0.8em;
931931 margin: 0;
932932 }
933 - #siteNotice div,
934 - #siteNotice p {
935 - margin: 0;
936 - padding: 0;
937 - margin-bottom: 0.9em;
938 - }
 933+#centralNotice {
 934+ margin-bottom: 0.9em;
 935+}
 936+#localNotice {
 937+ margin-bottom: 0.9em;
 938+}
939939 /* Categories */
940940 .catlinks {
941941 border: 1px solid #aaa;
Index: trunk/phase3/includes/GlobalFunctions.php
@@ -2034,7 +2034,7 @@
20352035 $notice = '';
20362036 }
20372037 }
2038 -
 2038+ $notice = '<div id="localNotice">'.$notice.'</div>';
20392039 wfProfileOut( $fname );
20402040 return $notice;
20412041 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r67927moving styling from skin to extension per comment to r67784kaldari21:35, 12 June 2010
r67928moving centralNotice styling to extension per comment at r67784kaldari21:47, 12 June 2010
r706051.16wmf4: MFT r67784, r67928catrope22:51, 6 August 2010

Comments

#Comment by Catrope (talk | contribs)   20:36, 12 June 2010

IMO the Vector skin in core should not contain selectors for things added by extensions, such as #centralNotice.

#Comment by Kaldari (talk | contribs)   21:49, 12 June 2010

Good point, I've moved the Central Notice styling from the skin to the extension as of r67928.

#Comment by TheDJ (talk | contribs)   22:32, 12 June 2010

Hmm, you are deprecating 1 CSS id, and adding 2 new CSS ids. I think just adding #centralnotice, and keeping #sitenotice should be considered. Deprecating ids without good reason usually just breaks scripts.

#Comment by Kaldari (talk | contribs)   01:45, 13 June 2010

"#sitenotice" is being kept. The ids that are being deprecated are "#sitenotice div" and "#sitenotice p" (neither of which are defined in all the skins). These unspecific sub-selectors were causing unintended styling issues to central notice banners (which are also included in the sitenotice div along with local sitenotices). To resolve this, I created specific divs to identify central notices and local notices within the sitenotice div and now apply specific styling to each. In this way, we can style local sitenotices without also messing up the styling of central notices and vice versa. I retained the original #sitenotice container div for exactly the reason you are suggesting (to not break things), even though it is largely unnecessary now that there are more specific divs within it.

#Comment by MZMcBride (talk | contribs)   01:52, 13 June 2010

If any IDs / classes are being added, shouldn't they have a "mw-" prefix?

#Comment by Kaldari (talk | contribs)   01:53, 13 June 2010

To give you a better idea, here's what the HTML looks like:
<div id="sitenotice">
   <div id="centralNotice">...</div>
   <div id="localNotice">...</div>
</div>

Now, all 3 of these can be styled specifically, and we aren't using generic selectors that apply (unintentionally) to every p and div in the banners.

#Comment by Kaldari (talk | contribs)   02:11, 13 June 2010

@MZMcBride: #centralNotice has already been in use (by the extension) and isn't a new addition. I would use #mw-localNotice, but it would seem odd since we're using #siteNotice and #centralNotice for the other pieces. Perhaps we could change them all at some point, although this would necessitate updating a hundred or so centralNotice banners. Which is worse: Having the names be inconstant with each other or inconstant with the convention?

#Comment by Tim Starling (talk | contribs)   08:35, 20 August 2010

The issue is that IDs in the skin can conflict with the IDs used to link to a specific section inside wikitext. But #localNotice will do for now.

Status & tagging log