Skip to content

Conversation

@Kobzol
Copy link
Member

@Kobzol Kobzol commented Oct 24, 2025

Create a separate page for each Rust Team member, which lists the teams they are a member (or alumni) of. This can be helpful to quickly show that someone is a part of the Project, e.g. when looking for a job.

The commit that adds Rayon reduces the render time from ~10s to ~2s on my PC, it can be helpful when iterating on the website. But if you don't like including Rayon, I can get rid of it.

image

@senekor
Copy link
Contributor

senekor commented Oct 28, 2025

I'm ok with adding rayon.

What's the value exactly of these two new handlebars helpers? At first glance, seems like more complexity.

I wonder if clicking on a picture should also link to this new page? Right now it links to GitHub. That seems a little inconsistent to me if the name links somewhere else. There are explicit "GitHub: " links, it makes sense to me if those are the only ones that point to the actual github user profile.

@Kobzol
Copy link
Member Author

Kobzol commented Oct 29, 2025

What's the value exactly of these two new handlebars helpers? At first glance, seems like more complexity.

The value is just that I wanted to have a shared logic for it, rather than risking that it will diverge across different pages. Both are just strings, so it would be tricky to turn them into partials. I could make the anchor part of team-text, if you want.

I wonder if clicking on a picture should also link to this new page? Right now it links to GitHub. That seems a little inconsistent to me if the name links somewhere else. There are explicit "GitHub: " links, it makes sense to me if those are the only ones that point to the actual github user profile.

I was thinking the same, I agree.

@senekor
Copy link
Contributor

senekor commented Oct 29, 2025

The value is just that I wanted to have a shared logic for it, rather than risking that it will diverge across different pages.

I don't agree with the reasoning here. There's technically a lot of duplication in the templates. Creating a separate rust helper for each occurrence of duplication would lead to a lot of unnecessary complexity.

What about this? Looks like you forgot to update a place where the "team anchor helper" should've been used? If so, the helper actually made the risk of divergence worse. Somebody might update the helper, thinking that should take care of everything, and forget to search for places where the helper wasn't used.

I think it's better to just copy these snippets everywhere. When somebody updates a hardcoded URL path, it's very obvious that searching for other occurrences is necessary. And when looking at the template, you can see what it does instead of having to understand what a handlebars helper is and dig through the Rust code.

The two existing helpers contain logic that would actually be tricky and brittle to implement in the templating language. These proposed helpers don't have logic, they're just snippets. I'd rather do without them.

Actually, there is a tiny bit of logic in the PersonLinkHelper in the form of making the github username lowercase. That only seems to be necessary because the same thing is done when rendering the pages. Can we just get rid of both and render the github usernames with original casing in the path? Or what's the reason to force it to lowercase?

@Kobzol
Copy link
Member Author

Kobzol commented Oct 30, 2025

What about this? Looks like you forgot to update a place where the "team anchor helper" should've been used? If so, the helper actually made the risk of divergence worse. Somebody might update the helper, thinking that should take care of everything, and forget to search for places where the helper wasn't used.

Yes, it should have also been used there, which is IMO an argument for having the helper, so that we don't forget to update multiple places if the heading changes. But I don't feel too strongly about this - for me as a developer of the PR, it would ofc be the easiest to just copy-paste everything, I wanted to make the code easier for you to maintain into the future, which is why I introduced the helpers 😆 But if you see this differently, then there's no point in it, I'll remove the helpers.

Actually, there is a tiny bit of logic in the PersonLinkHelper in the form of making the github username lowercase. That only seems to be necessary because the same thing is done when rendering the pages. Can we just get rid of both and render the github usernames with original casing in the path? Or what's the reason to force it to lowercase?

The URLs are case sensitive. Some GitHub usernames are quite complex regarding casing, so I thought that normalizing them might be a better idea, if someone wanted to generate the links manually (e.g. my username on github is Kobzol, but I usually just write kobzol). If we don't think that this use-case is required, we can just depend on people finding out about these links through the website itself, in which case we don't need to normalize.

@Kobzol
Copy link
Member Author

Kobzol commented Oct 30, 2025

Removed the helpers and lowercasing, and added a link to the person page when clicking on avatars (except for the person page itself, there the avatar links to GitHub).

@senekor
Copy link
Contributor

senekor commented Oct 30, 2025

I just noticed that the language is not preserved when clicking on the new person links. e.g. when I'm here:

http://localhost:8000/fr/governance/teams/leadership-council/

and click on any person, the fr/ is removed and I'm on the English version of the website. The language should be preserved.

@Kobzol
Copy link
Member Author

Kobzol commented Oct 30, 2025

Sigh, Handlebars doesn't report that the baseurl variable was unset :( I don't like this templating system.

@senekor
Copy link
Contributor

senekor commented Oct 30, 2025

I don't like this templating system.

That makes two of us!! 😂

@Kobzol
Copy link
Member Author

Kobzol commented Nov 4, 2025

Let me know if there's anything else I can change! :)

@senekor
Copy link
Contributor

senekor commented Nov 4, 2025

Oops, sorry. I didn't notice the new commit.

@senekor senekor merged commit cd41a39 into rust-lang:main Nov 4, 2025
2 checks passed
@Kobzol Kobzol deleted the people branch November 4, 2025 14:14
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.

2 participants