mhorvath2161 wrote:

unified diff file

Attached:

mhorvath2161 wrote:

I'm having trouble adding attachments. I keep getting an "was unable to add attachment, please use the 'create attachment' link" message.

You've just commented out code here and recommended uncommenting if someone wants to support it.

This is bad style, this requires people to modify core code just to enable a feature already inside of core. We make every effort to make sure people can make use of MediaWiki WITHOUT needing to modify code. This code change merely makes it harder for people to update if they want to use a feature inside of core. That alone makes me recommend WONTFIX.

Additionally as I commented on the other bug:

I don't like this idea. Just because we don't support it now, doesn't mean that
we won't support it in the future, and that extensions won't make use of it.
Show/Hide even supports it.

Personally, I just started using Drupal for my own portfolio, and I noticed
that drupal has a nice feature with table headers. I don't know if this is JS,
or browser built in. But when you scroll past the top of the table, the header
follows at the top of the page so you always know what a column is about. That
kind of thing makes me feel like finding a way to make it possible to use table
headers inside of MediaWiki and make that feature possible.

mhorvath2161 wrote:

IIRC, the THEAD element is not supported by Wikimedia software in any form, even if specifially added by hand.

The alternate row coloring can be re-enabled by setting the ts_alternate_row_colors parameter to equal 'true'.

ayg wrote:

-var ts_alternate_row_colors = true;
+var ts_alternate_row_colors = false;

This is a good change. It should be possible to re-enable this in MediaWiki:Common.js if necessary, but this should be off by default. I've been told it can cause noticeable lag for large tables. If we do want to support such classes, it should be done in the software, not client-side after the page has already been served.

• ts_alternate(table);

+ if (ts_alternate_row_colors) {
+ ts_alternate(table);
+ }

. . . and this is an even better change, the old behavior should qualify as something of a bug. I've committed these two changes in r40311.

The other part I don't see a need for. Does it slow anything down? If not, we may as well leave it in case we need it later.

mhorvath2161 wrote:

I thought of a better idea: why not enable it using a class name? This gives users control over it instead of devs.

However, AFAIK, the code doesn't actually *do* anything. Are there any skins that style the "even" and "odd" classes differently?

mhorvath2161 wrote:

(In reply to comment #5)

If we do want to support
such classes, it should be done in the software, not client-side after the page
has already been served.

Woops. Missed this bit. If you do it server-side, then the coloring will be all wrong when the table gets (re-)sorted.

Otherwise, I've seen people implement alternate row coloring using templates, and I'd like the script to be as fast as possible.

ayg wrote:

(In reply to comment #6)

I thought of a better idea: why not enable it using a class name? This gives
users control over it instead of devs.

Not sure if it's practical right now to parse classes server-side . . .

However, AFAIK, the code doesn't actually *do* anything. Are there any skins
that style the "even" and "odd" classes differently?

I don't think so, but it's potentially useful functionality. I wouldn't put a high priority on making it work, though, there are lots of more useful things to do.

(In reply to comment #7)

Woops. Missed this bit. If you do it server-side, then the coloring will be all
wrong when the table gets (re-)sorted.

Hmm, right. The sorting script will still have to recompute them when you sort. But it shouldn't have to on page load. It might be okay if it takes an extra second when the user explicitly chooses to push a button, if that's the cost of particular functionality, but it's not okay if it takes an extra second for the page to load.

mhorvath2161 wrote:

(In reply to comment #8)

(In reply to comment #6)

I thought of a better idea: why not enable it using a class name? This gives
users control over it instead of devs.

Not sure if it's practical right now to parse classes server-side . . .

No, no... The class name would be used to activate the script client-side. This is exactly how the "sortable" class works currently. I.e., the "sortable" class is ignored by the server; the javascript is what picks up on this and then makes the tables sortable.

(In reply to comment #7)

Woops. Missed this bit. If you do it server-side, then the coloring will be all
wrong when the table gets (re-)sorted.

Hmm, right. The sorting script will still have to recompute them when you
sort. But it shouldn't have to on page load. It might be okay if it takes an
extra second when the user explicitly chooses to push a button, if that's the
cost of particular functionality, but it's not okay if it takes an extra second
for the page to load.

Well, I would personally rather wait an extra second for the page to page load than wait a second each a button is clicked. UI latency is a bigger issue than loading times, IMO.

mhorvath2161 wrote:

I forgot to add that there's a variable called SORT_COLUMN_INDEX that isn't being used anywhere as far as I can tell. I think it can be safely removed as well.

mhorvath2161 wrote:

Also, the sort icons themselves have this bit of code attached to them:

onclick="ts_resortTable(this);return false;"

Is the "return false" portion necessary for some reason?

ayg wrote:

(In reply to comment #9)

No, no... The class name would be used to activate the script client-side.

Only if the addition of the row coloring on initial view is done client-side, which I don't think is acceptable.

Well, I would personally rather wait an extra second for the page to page load
than wait a second each a button is clicked. UI latency is a bigger issue than
loading times, IMO.

It's not either-or. You'll *always* have to wait when the button is clicked (modulo any optimizations that can be made). On the other hand, there's no reason you have to *also* wait when the page loads.

As I said, though, I wouldn't worry about it, since there doesn't seem to be much demand for it.

(In reply to comment #10)

I forgot to add that there's a variable called SORT_COLUMN_INDEX that isn't
being used anywhere as far as I can tell. I think it can be safely removed as
well.

Removed this in r40322, thanks.

(In reply to comment #11)

Also, the sort icons themselves have this bit of code attached to them:

onclick="ts_resortTable(this);return false;"

Is the "return false" portion necessary for some reason?

To avoid the icons doing anything else when you click them, I assume, like jumping the screen to the top of the page.

The whole sorted table thing was just copied verbatim from another website.
That's why it has a whole lot of useless irrelevant code. ts_alternate() should
be removed entirely, not disabled. The THEAD stuff is harmless and can be left
in.

mhorvath2161 wrote:

(In reply to comment #13)

The whole sorted table thing was just copied verbatim from another website.
That's why it has a whole lot of useless irrelevant code. ts_alternate() should
be removed entirely, not disabled. The THEAD stuff is harmless and can be left
in.

I'd rather comment it out. It can always be uncommented when Mediawiki adds support for the THEAD element. As it is now it's just dead weight.

mhorvath2161 wrote:

(In reply to comment #12)

(In reply to comment #9)

Is the "return false" portion necessary for some reason?

To avoid the icons doing anything else when you click them, I assume, like
jumping the screen to the top of the page.

Correct. However, this is only necessary due to the icon being surrounded by <a> tags. Why are these tags necessary? If a browser doesn't support the "onclick" attribute of the icon itself, then it would almost certainly choke on some other part of the script.

ayg wrote:

(In reply to comment #14)

I'd rather comment it out. It can always be uncommented when Mediawiki adds
support for the THEAD element. As it is now it's just dead weight.

There's no point in commenting it out. We're using version control here, it's not like we can't retrieve it later if we delete it.

(In reply to comment #15)

Correct. However, this is only necessary due to the icon being surrounded by
<a> tags. Why are these tags necessary? If a browser doesn't support the
"onclick" attribute of the icon itself, then it would almost certainly choke on
some other part of the script.

Well, it's semantically a link. This will give correct behavior in terms of the cursor turning into a finger, for instance. That can be emulated using CSS, but there may be other things associated with a link that can't. It's best to use the correct semantic markup.

mhorvath2161 wrote:

Could the onclick be removed from the image/span and the href attribute of the anchor changed to href="javascript:do_this_function();return false;"?

ayg wrote:

We don't like using JavaScript URLs, mostly. This is a good principle if we have a fallback non-JS URL (href can be set to non-JS content, onclick used for JS content), but we don't here, and can't, so I'm not sure why we would want to either use it or avoid it in this case. Why do you suggest changing the onclick to an href? It seems like there's no difference. For the informative display in the lower left of the browser or whatever?

mhorvath2161 wrote:

Moving the function call to the HREF attribute has the result of eliminating the need for the SPAN element. The leftover "sortarrow" class could then be applied to either the anchor or the image.

ayg wrote:

Okay, so is there any more unused code that needs to be disabled, or can this bug be marked FIXED?

Marking fixed per comment 20.