Skip to content

Add support for GitLab, Gitee, and SourceForge#41

Merged
Claudiohbsantos merged 12 commits intomaterial-extensions:masterfrom
csandman:gitlab-gitee-providers
Jul 17, 2022
Merged

Add support for GitLab, Gitee, and SourceForge#41
Claudiohbsantos merged 12 commits intomaterial-extensions:masterfrom
csandman:gitlab-gitee-providers

Conversation

@csandman
Copy link
Contributor

@csandman csandman commented Jul 1, 2022

As the title says, this PR adds support for GitLab, Gitee, and SourceForge, and would close #2. GitLab was originally requested in that issue, but the other two were not. Like I had mentioned in a comment, I've looked through a bunch of these extensions, and some of them have support for Gitee and some have support for SourceForge, so I decided to try adding both of them. The results look pretty good IMO.

Before I talk about those, I made two other small general tweaks that I'd like to point out.

  1. I switched the filename getter to take the first part of a path (split on /) if the name is representative of a path. For example, when there's only one folder inside another folder, GitHub concatenated the names and clicking sends you straight through to the second folder. In general, I've found that the top level folder in these situations is more likely to have a custom icon, so I used the first part of the path. However, it could technically make more sense to use the end part of the path, as that's where the link goes. Either way, up to you whether you want to keep this change.

image

  1. In order to get these new providers working, I changed the structure of the getIsDirectory, getIsSubmodule, and getIsSymlink functions to accept an object with the shape { row, icon }. In some cases, you can not determine these features from the icon itself, and having access to the row element gives a lot more freedom for figuring that out. It was possible to get the same features from the row by chaining .parentNode but I've never felt super comfortable with traversing up a DOM tree. Plus, it's still about as simple to implement these functions as you can just destructure the part you need:
getIsDirectory: ({ icon }) => icon.getAttribute('data-testid') === 'folder-icon',
getIsSubmodule: ({ row }) => row.querySelector('a')?.classList.contains('is-submodule') || false,

So here's some info on the new providers:

GitLab

Like I mentioned, the main purpose of adding GitLab support is so that the folders get icons. Besides that it is mostly similar.

image

One extra feature I added however, was replacing the icon for the Readme shown below the list of files:

image

As well as the icon you see when viewing a specific file:

image

I'm sure you have plenty of repos you can test this with as you mentioned you're now using GitLab for work, but here's an instance of that test repo I shared with you that I pushed to GitLab: https://gitlab.com/csandman/test

Gitee

I'm not gonna lie, I had never heard of Gitee until I saw another extension supporting it, but it has a very similar layout to GitLab. I did the same replacements as I did on GitLab, the file table, readme title, and individual file titles:

image

image

I tested the symlink/submodule getters using that same repo but I don't feel like making it public because they have an approval process for that and I don't want to deal with it. You can test it with any of the repos on their explore page: https://gitee.com/explore

SourceForge

For SourceForge, I had to handle things a little differently than the rest. Each project has both a code page as well as a files page with a list of downloadable output files. I decided both would benefit from this replacement. However, on the Files page, only the folders have an icon by default, so I targeted the element wrapping the icon, replaced the child icon if it does exist, and prepended the new icon if it does not.

Here's the Files page:

image

And here's the Code page:

image

You can check out the code page for my test repo mentioned above here: https://sourceforge.net/p/csandman-test/code/ci/main/tree/

And you can check the files page for any of the public projects listed here: https://sourceforge.net/directory/

Let me know if anything needs changing!

@Claudiohbsantos Claudiohbsantos mentioned this pull request Jul 2, 2022
Copy link
Member

@Claudiohbsantos Claudiohbsantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for another extremely clean PR! Your descriptions are very easy to follow.

I answered most things in individual comments so I'll skip them here. And I really like the update to the signature of getIsDirectory and family.

Only merge blocking changes are the ones related to the selectors feeling a bit too wide. Let me know if any of them are wide for a particular reason though. Everything else looks great!

@csandman
Copy link
Contributor Author

csandman commented Jul 2, 2022

Thanks for checking it out! I can't remember the exact reasons of the top of my head but some of the selectors are the way they are because of issues with more specific ones not matching properly. I'll check them out and reply to the specific comments with my thought process on Tuesday!

csandman and others added 2 commits July 1, 2022 23:34
Co-authored-by: Claudio Santos <Claudiohbsantos@users.noreply.github.com>
Co-authored-by: Claudio Santos <Claudiohbsantos@users.noreply.github.com>
@csandman
Copy link
Contributor Author

csandman commented Jul 2, 2022

Also, I can't tell if from your comments you're saying you'd like to avoid the icons related to the file headers? If so, that would probably simplify a lot of the selectors, as that's a large part of why they're more general. So let me know if that's the case, I have no problem with removing them!

@Claudiohbsantos
Copy link
Member

Thanks, no rush on this!

And I like the file header icons, no need to remove them. I was mostly thinking of just setting up some basuc hierarchy in the selectors so they only match icons within the tables/rows/container that we expect. We probably don't even need to necessarily change the selectors, just constrain them to only match within certain containers. Not sure if that makes sense, let me know if I'm talking non sense and I can try to explain better

@csandman csandman requested a review from Claudiohbsantos July 5, 2022 18:46
@csandman
Copy link
Contributor Author

csandman commented Jul 5, 2022

Alright I made the changes you requested, in a way that I hope works for you! I get what you mean now with only matching within the containers the icon/filename are expected to be in. Like i mentioned in one of my comments, I hadn't realized you could use top level selectors when running a query selector on a specific element, and doing so should probably help to make sure things are matching correctly.

On a side note, one thing I've tried to avoid with making selectors too too specific is that this could also lead to increased maintenance, if the site changes its structure at all, which could make the replacement stop working for existing icons. I feel like this is generally more likely than a change that causes the extension to replace icons where you don't intend it to.

But yeah, let me know if these new selectors are more along the lines of what you meant or if there are any other changes you want made to them!

@csandman
Copy link
Contributor Author

csandman commented Jul 8, 2022

One last tiny change I added, with the more specific GitHub selectors added previously, the symlink/submodule icon was not being identified properly, so I added one more selector to that list to make it work!

I noticed it when looking at this repo (in case you want to see): https://github.com/sandreas/m4b-tool

Copy link
Member

@Claudiohbsantos Claudiohbsantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, your notes about selectors in the different cases make sense to me, thanks for looking into these again!

This looks great to me! I'll mark it as approved since I don't expect any more changes to be necessary. I'll hold off on merging for a bit just so I can batch the changes that will affect permissions all together in order to avoid disabling the extension for users repeatedly, but I'll try to get this out this week.

@Claudiohbsantos Claudiohbsantos merged commit afa8fe4 into material-extensions:master Jul 17, 2022
@csandman csandman deleted the gitlab-gitee-providers branch July 18, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gitea & GitLab support

2 participants