Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use new design for FileContributors #14898

Open
wants to merge 34 commits into
base: dev
Choose a base branch
from

Conversation

JoeChenJ
Copy link
Contributor

@JoeChenJ JoeChenJ commented Feb 13, 2025

Description

Use new design for FileContributors

Related Issue

#8024 & #14473

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program labels Feb 13, 2025
Copy link

netlify bot commented Feb 13, 2025

Deploy Preview for ethereumorg failed.

Name Link
🔨 Latest commit 0319ac9
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/67eacc33c5708e0008f3465f

@JoeChenJ
Copy link
Contributor Author

Hey @konopkja, could you take a look at this?

Let me know if anything needs improvement!

@konopkja
Copy link
Contributor

Screenshot 2025-02-14 at 9 09 23

looks awesome!

I think this gap could be 50% of what it is now, but overall its great!


I noticed that the component is only visible on desktop, do you intend on preparing it for mobile also? I think there is enough space

@JoeChenJ
Copy link
Contributor Author

Screenshot 2025-02-14 at 9 09 23

looks awesome!

I think this gap could be 50% of what it is now, but overall its great!

I noticed that the component is only visible on desktop, do you intend on preparing it for mobile also? I think there is enough space

@konopkja Sure, I'll work on this soon!

@JoeChenJ
Copy link
Contributor Author

Hey @konopkja , I’ve adjusted the gap and made the updates for mobile. I also want to point out a few additional changes I made that you might not notice:

  1. I added a “Translator” tag next to the Crowdin contributors.

  2. I changed the redirect logic for Crowdin contributors — they now link to their Crowdin profiles instead of GitHub.

  3. On pages with Crowdin contributors, the modal now displays both Crowdin and GitHub contributors (it used to show only one or the other).

Could you take a look at those as well and let me know if everything looks fine? Appreciate your feedback!

@konopkja
Copy link
Contributor

Hey @konopkja , I’ve adjusted the gap and made the updates for mobile. I also want to point out a few additional changes I made that you might not notice:

  1. I added a “Translator” tag next to the Crowdin contributors.
  2. I changed the redirect logic for Crowdin contributors — they now link to their Crowdin profiles instead of GitHub.
  3. On pages with Crowdin contributors, the modal now displays both Crowdin and GitHub contributors (it used to show only one or the other).

Could you take a look at those as well and let me know if everything looks fine? Appreciate your feedback!

Hey @JoeChenJ it looks perfect! Really appreciate this particular contribution <3

@konopkja
Copy link
Contributor

shall we discuss extending this component to other pages outside developer docs or would it be better tackled in another PR?

@JoeChenJ
Copy link
Contributor Author

shall we discuss extending this component to other pages outside developer docs or would it be better tackled in another PR?

I think it would be best to merge this PR first, and then we can extend it in another one, as it might take a bit more time

@JoeChenJ JoeChenJ marked this pull request as ready for review February 14, 2025 12:42
@JoeChenJ
Copy link
Contributor Author

Hey @konopkja, I've made some progress on the extension—the component is now available at the end of the markdown pages.

Since this PR is still open, I've pushed the changes here!

@JoeChenJ
Copy link
Contributor Author

619627a370888e52a2a691864f1191f
bb3ae600388010b97db61d4c09deb6e
Hey @konopkja for React pages, does placing the component here look good to you?

@nloureiro
Copy link
Contributor

@JoeChenJ, great contribution! Thank you

From my side, this component should always be at the end of the content, before the callouts and widgets.

I would add a border-top to it. See if this screenshot helps.

Screen Shot 2025-02-17 12 35 45 AM

Are you thinking of all content pages? Something like all pages minus (homepage & Hubpages). Is this the goal?

@JoeChenJ
Copy link
Contributor Author

@nloureiro thank you for the guidance! Adding a border-top looks much better.

I was planning to add this component on every page, but not sure if it fits well across all designs. Do you think we should exclude certain pages? Would love to get your thought on it!

@JoeChenJ
Copy link
Contributor Author

Hey @nloureiro @konopkja , I've added this component on the following React pages:

  1. what-is-ethereum
  2. learn
  3. run-a-node
  4. eth
  5. dapps
  6. get-eth
  7. gas
  8. bug-bounty
  9. wallets
  10. staking
  11. roadmap/vision
  12. layer-2/learn

Could you please review them and let me know if I need to remove it from some pages/add it on more pages, or tweak its position on certain pages? Thank you!

@konopkja
Copy link
Contributor

konopkja commented Feb 20, 2025

Screenshot 2025-02-20 at 11 09 03

guess its ok, but why is the line in dark mode so strong? noticed some other components have stronger border in ur PR also while in prod are weaker.

should be "background medium token" which should be "#333333"

@JoeChenJ
Copy link
Contributor Author

Screenshot 2025-02-20 at 11 09 03 guess its ok, but why is the line in dark mode so strong? noticed some other components have stronger border in ur PR also while in prod are weaker.

should be "background medium token" which should be "#333333"

That's wierld, I think the changes in #14931 lead to this problem

@JoeChenJ
Copy link
Contributor Author

should be fine now

@corwintines corwintines self-assigned this Mar 24, 2025
Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

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

Great job on this so far @JoeChenJ!

I just have a couple changes, and one note.

I think for consistency sake across the site, we should have this above the feedback card. I think the pages I noticed where it wasn't and it looked odd for me was:

  • /what-is-ethereum
  • /run-a-node
  • /get-eth
  • /gas
  • /wallets

There may have been some conflicting feedback on this before, but chatted with @nloureiro and @konopkja today to confirm this.

Thats the main design change for this, but I have one note to add to let you know about. We will be bringing in #15114 shortly, which changes the path for these react pages. I want to bring that PR in first, and then check if we need to make any changes to the getPageContributorInfo function in @/lib/utils/contributors.ts.

Overall, this is great and excited to get this in. For me just the little design cleanup, and then spot checking the functionality after that other PR is brought in.

@wackerow
Copy link
Member

Second that...

Will try to help add clarity on the changes the above PR will bring.

Page components are getting moved to a new location, but also being split into two files. One will be the base page (server component), and the other will contain any client-side logic.

By example, let's look at the bug-bounty page:

  • Existing location would be at src/pages/[locale]/bug-bounty.tsx
  • With the App Router changes coming, this file is moved to app/[locale]/bug-bounty/page.tsx.
  • Simultaneously, the component logic has been extracted to app/[locale]/bug-bounty/_components/bug-bounty.tsx.

@pettinarip Correct me if I'm wrong, but to my understand the existing page components (src/pages/[locale]/bug-bounty.tsx) was git mv'd to the _components subdirectory (app/[locale]/bug-bounty/_components/bug-bounty.tsx) so the history came with it... and the page.tsx component was created new... as such, it should be sufficient to merge the git commit history from these two new files to get the correct resulting list of contributors.

@JoeChenJ
Copy link
Contributor Author

Second that...

Will try to help add clarity on the changes the above PR will bring.

Page components are getting moved to a new location, but also being split into two files. One will be the base page (server component), and the other will contain any client-side logic.

By example, let's look at the bug-bounty page:

  • Existing location would be at src/pages/[locale]/bug-bounty.tsx
  • With the App Router changes coming, this file is moved to app/[locale]/bug-bounty/page.tsx.
  • Simultaneously, the component logic has been extracted to app/[locale]/bug-bounty/_components/bug-bounty.tsx.

@pettinarip Correct me if I'm wrong, but to my understand the existing page components (src/pages/[locale]/bug-bounty.tsx) was git mv'd to the _components subdirectory (app/[locale]/bug-bounty/_components/bug-bounty.tsx) so the history came with it... and the page.tsx component was created new... as such, it should be sufficient to merge the git commit history from these two new files to get the correct resulting list of contributors.

Unfortunately, the commit history didn't carry over to the new path, if we merge app/[locale]/bug-bounty/page.tsx and app/[locale]/bug-bounty/_components/bug-bounty.tsx, we are only getting history starting from #15114 🫠

@JoeChenJ JoeChenJ force-pushed the Use_new_design_for_FileContributors branch 2 times, most recently from 1240255 to 0319ac9 Compare March 31, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants