Skip to content

Conversation

@Kobzol
Copy link
Member

@Kobzol Kobzol commented Nov 14, 2025

This PR adds a dedicated Funding page that lists all active Project members that have GitHub Sponsors enabled and have opted into it being publicized through team. The order of people is randomized using JavaScript on every page load, to reduce ordering bias.

Maybe we could even add a dedicated "Funding" link in the top-level navbar?

image image

@Kobzol Kobzol marked this pull request as ready for review November 15, 2025 17:00
@Kobzol Kobzol requested a review from a team as a code owner November 15, 2025 17:01
@Kobzol
Copy link
Member Author

Kobzol commented Nov 15, 2025

Rebased.

Copy link
Contributor

@senekor senekor left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I don't like the handling of the custom script though. This is basically adding a hook where every page can inject it's custom javascript, right? Why not just write it inline inside the "page" block? I checked if there's a precedent for this. tools-install.js is another custom script that only applies to one page. But it seems to me that's handled in a complicated way too... it get's another generated name "vendor_<random_num>.js". I can't see a reason why this is done. I would prefer if we put all scripts in static/scripts/ and reference them explicitly with the same name as the source file in HTML templates. What do you think?

@senekor
Copy link
Contributor

senekor commented Nov 15, 2025

And once this is merged, I imagine it would be appropriate to make a post on the inside-rust blog to let contributors know they can add themselves to this list.

@Kobzol
Copy link
Member Author

Kobzol commented Nov 15, 2025

This looks pretty good. I don't like the handling of the custom script though. This is basically adding a hook where every page can inject it's custom javascript, right? Why not just write it inline inside the "page" block? I checked if there's a precedent for this. tools-install.js is another custom script that only applies to one page. But it seems to me that's handled in a complicated way too... it get's another generated name "vendor_<random_num>.js". I can't see a reason why this is done. I would prefer if we put all scripts in static/scripts/ and reference them explicitly with the same name as the source file in HTML templates. What do you think?

The main reason why I added the script section is because when I added the script to the end of the page inline, it broke the logic I added in #2231, because the last section was no longer the last child of its parent, as there now was the <script> 😆 In general, I think it's slightly better if JS scripts are at the very end of the page, rather than being in some div inside the page.

I'm fine with adding the logic into an external file, although it makes the logic a bit more distributed. When it's in the same file, it's easier to see that you should update the script when the HTML IDs/classes change, for example.

@Kobzol
Copy link
Member Author

Kobzol commented Nov 15, 2025

And once this is merged, I imagine it would be appropriate to make a post on the inside-rust blog to let contributors know they can add themselves to this list.

I wanted to leverage the new Project Zulip channel that we should have hopefully soon (rust-lang/team#1969) to let people know, but a blog a post would also work, yeah. I first wanted to have something to show for it, to motivate people to add it, when they see this new page.

@senekor
Copy link
Contributor

senekor commented Nov 15, 2025

The main reason why I added the script section is because when I added the script to the end of the page inline, it broke the logic I added in #2231, because the last section was no longer the last child of its parent

You could sqeeze it inside the <section> :)

I think it's slightly better if JS scripts are at the very end of the page, rather than being in some div inside the page.

Why?

I'm fine with adding the logic into an external file, although it makes the logic a bit more distributed. When it's in the same file, it's easier to see that you should update the script when the HTML IDs/classes change, for example.

Fair. In that case, we should inline the tools-install.js file as well? (btw. that script is currently included somewhere in the middle of the html body, not the end)

@Kobzol
Copy link
Member Author

Kobzol commented Nov 15, 2025

Why?

When JS is encountered in the middle of the page, it will block the processing of further page contents until the JS is parsed and processed. Granted, here it probably doesn't matter at all, as the script is tiny and uses DOMContentLoaded anyway. I started doing web stuff somewhere around 2010, so I suppose I'm still quite old-fashioned with some things 😆 Back at that time it was common to stuff all JS towards the end of the page because of this.

Anyway, no problem with keeping it before the end of the section. Wrapping it in a <section> seems like a bit of an unnecessary hack, so I like keeping it within the section more. Done that.

Fair. In that case, we should inline the tools-install.js file as well? (btw. that script is currently included somewhere in the middle of the html body, not the end)

Good question. I think there are two counter-arguments to that, in theory:

  • If the script is quite large, it can be more difficult to read it when it's embedded in the Handlebars template. Especially if we don't have a separate script block, but we have to move it in the middle of HTML, so that it will be nested and indented unnecessarily. I don't think that this is the case for tools-install.js though.
  • If the JS is inlined, it won't be cached as well as if it was an external file. That being said, this file is small, so it probably doesn't matter.

So yeah, if you want, I can inline the tools-install.js file.

@senekor
Copy link
Contributor

senekor commented Nov 15, 2025

I think I prefer the script in a separate file in static/scripts/ after all. I don't want javascript sprinkled over multiple directories and file types. If all significant javascript is in one directory, it's easier to get an overview over everything the website is doing on the client side. (trivial stuff like the redirect template is obviously fine)

And as you said about tools-install.js, having javascript in its own file has benefits. I'll add to the list that it's easier for language tooling to recognize standalone files, e.g. syntax highlighting & linting.

The part about being careful to update IDs that are referenced in different files is true, but it comes with the territory of web stuff. And we already have many different html.hbs, css, and js files, so that train has kinda sailed. Contributors and reviewers just have to grep the entire repo when changing a static string, there's no way around that.

@senekor
Copy link
Contributor

senekor commented Nov 15, 2025

Could it be that the generated name for the tools-install.js file is intended to break caching? To force a reload of the file in case it changed?

@Manishearth
Copy link
Member

I think I prefer the script in a separate file in static/scripts/ after all. I don't want javascript sprinkled over multiple directories and file types. If all significant javascript is in one directory, it's easier to get an overview over everything the website is doing on the client side. (trivial stuff like the redirect template is obviously fine)

I agree, we should do this.

@Kobzol
Copy link
Member Author

Kobzol commented Nov 16, 2025

Could it be that the generated name for the tools-install.js file is intended to break caching? To force a reload of the file in case it changed?

Yup, pretty sure that's the reason.

Created a separate JS script for the funding page.

I also removed the "app" JS file, since I think that it doesn't make sense to combine the funding JS (as the scripts might not be self-contained and shouldn't run on arbitrary pages) and the tools installation JS, and instead created a separate JS file for both.

@senekor senekor merged commit 7353904 into rust-lang:main Nov 17, 2025
2 checks passed
@Kobzol Kobzol deleted the funding-page branch November 18, 2025 06:54
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.

3 participants