... maybe we should also add a class to each item.

@Yaron_Koren may i take up this issue?

Definitely!

@Yaron_Koren I installed the mediawiki repo and in the extensions folder i installed the inline comments repo however I am not able to locate the LocalSettings.php file for configuration. Did i make any mistake while setting up?

Is the wiki running? If so, LocalSettings.php is in the right place - it should be right within the main MediaWiki folder.

@Yaron_Koren I was successfully able to set it up and install the inline Comments extension , but I am not able to locate the ALrow.php file. Can you assist me on that?

Sorry, I didn't realize you were talking about InlineComments before. This is not a task for the InlineComments extension - it's for the Admin Links extension.

Change 1007973 had a related patch set uploaded (by Rockingpenny4; author: Rockingpenny4):

[mediawiki/extensions/AdminLinks@master] ALRow: Add row search class

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

@Yaron_Koren please review the patch

@Rockingpenny4 - thanks for this patch. Well, this is the code that was originally suggested - but it won't fully work. Class names can't contain a space for example, and a quotation mark will close the class name in the HTML, so if the row name contains either a space or a quotation mark, this will fail. (There may be other characters that will also break things.)

Thinking more about it now, I'm not sure that a class name (as was originally suggested) is the way to go. It may be better to make this a custom attribute, like "data-row-name=", rather than "class=". (Custom attribute names have to start with "data-".) For custom attributes, there are (if I understand it correctly) many fewer rules, or even no rules at all - so you can have spaces, commas, etc. What do you think? Does that make sense?

@Yaron_Koren - Yes that is definitely a much better way to implement that , custom attributes don't seem to have any rules and are a well fit for this case . Does something like this look correct

$text = "	<p data-row-name='admin-links-row-" . $this->name . "'>\n";

No - I would say the "admin-links-row" part is unnecessary, if it's in that attribute. Also, a single quote, i.e. apostrophe, will still break the HTML, like it would with the original patch. (I know I said quotations before, but I should have said apostrophes.)

What if we were to escape the special characters using htmlspecialchars?

Whatever works is good. :)

I should note that another option is to use MediaWiki's built-in Html class - which is probably the most "correct" approach, though of course not the only one.

@Yaron_Koren where can i find more information regarding the built-in classes , should I refer to this https://www.mediawiki.org/wiki/Manual:Html.php?

Sure, that looks like a good resource. It could also help to look at the file itself.

$text = Html::rawElement(
    'p',  ['class' => $this->name], "\n"
);

On going through the documentation and the file , maybe the rawElement() method could be used as above for the classnames.

Using rawElement() does seem like a reasonable approach.

Great, should I go ahead and push the changes?

Sure - it would be great to see what you've come up with.

@Yaron_Koren I have updated the original patch and while testing it locally, I created an instance of a row with name containing spaces but I wasn't able to explicitly access the class attribute's value .

@Yaron_Koren I have updated the original patch and while testing it locally, I created an instance of a row with name containing spaces but I wasn't able to explicitly access the class attribute's value .

@Yaron_Koren I tested row names with spaces and special characters and it breaks with apostrophes so either we can specify to replace apostrophes with something else in classnames or we can use htmlspecialchars to escape them .

Actually, it looks like apostrophes are being handled fine - they're getting HTML-escaped, which I think is valid behavior. (That is, I think JS and CSS selectors will still find it.)

Okay then, are there any more changes to be made? If not, I will look into the InlineComments codebase and resolve some of the bigger issues.

Are spaces handled correctly? That was the big reason to use an attribute and not a class. Also, does the HTML in general look correct?

I have attached screenshots above when spaces are included, it replaces spaces with underscore in class names and the website and the source HTML renders fine, should I attach a screenshot of that as well?

Oh, great. Yes, a source of the HTML would be good - especially for a row that contains items.

I changed the approach a bit as in the earlier approach, rawElement self opened and closed the p tag and another p tag was manually appended resulting in two p tags. In the new approach all the content is collected first inside the loop and passed as an argument. This ensures that the <p> tag is properly closed with all the content inside it.

That sounds good - please update the patch!

@Yaron_Koren https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AdminLinks/+/1007973

Oh - I thought you had changed the patch; I guess not. If the patch didn't change, what did you mean by "the earlier approach" and "the new approach"?

Apologies for the inconvenience , I'm still figuring out gerrit , it looks like I made the changes but didn't click publish, here are the new changes : https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AdminLinks/+/1007973/3 .

@Rockingpenny4 - great, this looks like a reasonable approach to use.

Two things:

• Please use tabs instead of spaces.
• Using simply the row name as the class name is not a good solution, I don't think - there's a high chance that a row name like "main" will conflict with some other class called "main". I think there are only two real approaches: having it as a class name with a prefix like "admin-links-row-", or having it as a custom attribute like "data-row-name". They both have their advantages; I'm not sure which is the better solution. What do you think?

@Yaron_Koren
I will definitely use tabs and make the indentation changes , both approaches will work but I would prefer custom attributes as it would make it easier for future developers to maybe add more information to rows and also convenient for designers.

That sounds logical.

@Yaron_Koren I have made the required changes and tested them locally and updated on the patch.

This generally looks good! But there's something wrong with the tabs and spaces, as you can see in the diff.

Also, why did you pick "data-row" as the attribute name?

In T327263#9596129, @Yaron_Koren wrote:

@Rockingpenny4 - great, this looks like a reasonable approach to use.

Two things:

• Please use tabs instead of spaces.
• Using simply the row name as the class name is not a good solution, I don't think - there's a high chance that a row name like "main" will conflict with some other class called "main". I think there are only two real approaches: having it as a class name with a prefix like "admin-links-row-", or having it as a custom attribute like "data-row-name". They both have their advantages; I'm not sure which is the better solution. What do you think?

I thought data-row-name was preferred , should I remove row from attribute name?

Right, we talked about "data-row-name", but now it's just "data-row". It's not a big change, and it's not necessarily bad - I was just wondering why you made the change.

I thought name was meant to be this->name of the row, so should I leave it as it is or add name? And I will fix the tabs and spaces.

No, the value of the attribute is $this->name. Anyway, leaving aside what was said before, what do you think a good name would be for this attribute? Ideally it should be simple but also non-ambiguous.

Any of 'data-rowName' , 'data-row-name' would be clear and simple enough I think.

That sounds fine with me - let's go with "data-row-name", then.

I have updated the changes for it.

I went through the pipeline failure and there were some linting errors and I tried to fix it using

phpcbf --standard=PSR12  extensions/AdminLinks/includes/ALRow.php

but on running composer test it still outputs error of using spaces instead of tab on line 45 and 46. How should I fix that error?

What specific errors are you seeing? I didn't see anything about using spaces instead of tabs.

@Yaron_Koren I resolved the errors and now the composer test is not showing any linting errors, I have updated the patch as well.

Change 1007973 merged by jenkins-bot:

[mediawiki/extensions/AdminLinks@master] ALRow: Add row search class

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

Thanks a lot for your help @Yaron_Koren . I previously made some contributions to WikiEduDashboard and found this project listed for GSOC quite interesting. Should I try my hands on some of the bigger tasks like T355943 to get a better understanding of the codebase or would you suggest some other tasks for me to take up?

Well, as far as tasks for GSoC applicants to show their coding skills, we trying to limit people to just one each, to make things fair. And that task you linked to is indeed bigger than a microtask. With that said, you (and anyone else) are of course free to work on whatever you want to, this being open source.

@Yaron_Koren , will my previous contributions to other projects like WikiEduDashboard under Wikimedia help my application in being accepted for this project?

Sure, it would be great to see your other contributions. Where can we see them?

https://github.com/WikiEducationFoundation/WikiEduDashboard/pull/5661
https://github.com/WikiEducationFoundation/WikiEduDashboard/pull/5601
https://github.com/WikiEducationFoundation/WikiEduDashboard/pull/5687
https://github.com/WikiEducationFoundation/WikiEduDashboard/pull/5581 [Draft]
These are some of the PR's that I have made for WikiEduDashboard .

Impressive! Thanks. (And I hope they get merged in!)