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

Create a /pro-help page to promote the Contractors program #1015

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jviotti
Copy link
Member

@jviotti jviotti commented Oct 10, 2024

@jviotti
Copy link
Member Author

jviotti commented Oct 10, 2024

@benjagm Who would be the best for me to ask some questions on how to create this page? I never used Next.js or this JavaScript-drive way of creating a website and I'm completely lost:

  • I have no idea how to read the contractors.json file from _include
  • I have no idea how to generate dynamic content based on it
  • Also struggling a bit to define layout

@jviotti
Copy link
Member Author

jviotti commented Oct 10, 2024

Here is what I have so far:

Screenshot 2024-10-10 at 11 31 26 am

I want to render a table or list showing the people that opted in for the program and then just link to this page from a bunch of places.

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Oct 12, 2024

Hey @jviotti If you don't mind I'd like to try to help you.

  • You could use the /tools page as a reference. It does both, read contents from a data file and renders a dynamic table from it.
  • To make the file contents available to the page, you'll need to use getStaticProps. This function passes the returned values as props to your component. Read docs

Let me know if you are still stuck!

@jviotti
Copy link
Member Author

jviotti commented Oct 13, 2024

Hi @DarhkVoyd , thanks a lot for the pointers 🙏🏻 Let me give it a shot and see how it goes.

@jviotti
Copy link
Member Author

jviotti commented Oct 22, 2024

@DarhkVoyd I'm giving it a shot, but seems like for some strange reasons, I cannot import fs on my new page, even though its exactly the same way as it is imported in /tools?

Screenshot 2024-10-22 at 10 57 33 am

Signed-off-by: Juan Cruz Viotti <[email protected]>
@DarhkVoyd
Copy link
Member

@jviotti Looks strange to me too, weird. I checked out your last commit and I am unable to reproduce this error. Can you please help me reproduce this? Maybe, this has to do something with your JavaScript Runtime, which runtime are you running this on?

@jviotti
Copy link
Member Author

jviotti commented Oct 22, 2024

@DarhkVoyd I was indeed able to fix it with my last commit. I'm fighting the layout/styling stuff right now. This is the design I created that I'm trying to implement now (a table didn't look very well):

Extra Large@2x

Do we have an icon library present in the project? Also, what is the CSS utility library this website is using? Any help on the above design very appreciated. I feel I have no idea of what I'm doing with Next.js 😅

@jviotti
Copy link
Member Author

jviotti commented Oct 22, 2024

All based on the JSON file we are starting to maintain here: https://github.com/json-schema-org/community/blob/main/programs/contractors/contractors.json

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Oct 22, 2024

  • What did you change in the last commit to fix this? I am intrigued to know what caused the error.
  • I think this card design does look better than a table.
  • As far as I'm aware, there is no icon library present in the project. The project hosts its own svg icons here.
  • We use Tailwind CSS within the project.
  • This first-iteration seems fine to me. You seem to do quite fine with Next.js.
  • Feel free to reach out to me on slack anytime you get stuck. I'd be more than happy to help!

@jviotti
Copy link
Member Author

jviotti commented Oct 22, 2024

What did you change in the last commit to fix this? I am intrigued to know what caused the error.

I had to use fs only within getStaticProps

@DarhkVoyd
Copy link
Member

If you'd like, I would be happy to collaborate with you to make tweaks and fine tune the styles after you complete the page during review.

@DarhkVoyd
Copy link
Member

What did you change in the last commit to fix this? I am intrigued to know what caused the error.

I had to use fs only within getStaticProps

Ohh! 😆 yes that would do it, getStaticProps and such get... prefix special method are only server side, all else is run in the browser context.

Signed-off-by: Juan Cruz Viotti <[email protected]>
@jviotti
Copy link
Member Author

jviotti commented Oct 22, 2024

@DarhkVoyd I would love to collaborate on it. Here is my very basic first take on it. If you want/can push any further improvements to it here, please do! 🙏🏻

Screenshot 2024-10-22 at 2 40 43 pm

@jviotti
Copy link
Member Author

jviotti commented Oct 22, 2024

Also cc @jdesrosiers , as you expressed interest in the contractor program. Any feedback?

@jdesrosiers
Copy link
Member

Looks like a good start! Thanks for moving this forward.

Signed-off-by: Juan Cruz Viotti <[email protected]>
Signed-off-by: Juan Cruz Viotti <[email protected]>
Signed-off-by: Juan Cruz Viotti <[email protected]>
@jviotti
Copy link
Member Author

jviotti commented Oct 24, 2024

@DarhkVoyd I made some minor tweaks, like linking to this from the main page, etc. Are there any fixes/polishes you would like to land (like making sure I didn't break the responsiveness?) At least from a content point of view, I'm happy enough to get it polished, do a team review, and see if we can get it live soon!

@DarhkVoyd
Copy link
Member

Screenshot 2024-10-26 at 2 42 21 PM
Screenshot 2024-10-26 at 2 46 15 PM
I did some minor tweaking viz. fixed some linting errors, I liked the icons from your first iteration so added them back and the mobile version looked a bit cluttered so made some layout changes. Let me know if they look fine to you!

@jviotti
Copy link
Member Author

jviotti commented Oct 26, 2024

Looks wonderful to me! I'll mark it as ready to review.

@jviotti jviotti marked this pull request as ready for review October 26, 2024 20:07
@jviotti jviotti requested a review from a team as a code owner October 26, 2024 20:07
@jviotti jviotti changed the title [WIP] Create a /pro-help page to promote the Contractors program Create a /pro-help page to promote the Contractors program Oct 26, 2024
@DarhkVoyd
Copy link
Member

@jviotti should I push the commit?

@jviotti
Copy link
Member Author

jviotti commented Oct 28, 2024

@DarhkVoyd Yes, feel free to push to this branch!

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8dc7e29) to head (31758ab).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1015   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          373       373           
  Branches        94        94           
=========================================
  Hits           373       373           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DarhkVoyd
Copy link
Member

@jviotti Hey, I hope you don't mind, I pushed my mobile layout changes along with some linting fixes that caused the build to fail. But, the CI for build preview is still failing as the community submodule responsible for the contractors.json is still pointing to an older commit.

@jviotti
Copy link
Member Author

jviotti commented Oct 28, 2024

@DarhkVoyd No worries, thanks for pushing it! :)

But, the CI for build preview is still failing as the community submodule responsible for the contractors.json is still pointing to an older commit.

I thought I had done it on #1014, unless I'm missing something specific to the way preview works?

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Oct 28, 2024

I checked the actions log, the submodule is still checking out an older commit. Maybe, to update submodules for the PR with the changes on main, we need to separately update the submodule on the PR. Makes sense?

  From https://github.com/json-schema-org/community
   * branch            b8c6a9ef4511bbf70de9ef0666987c9cad449b54 -> FETCH_HEAD
  Submodule path '_includes/community': checked out 'b8c6a9ef4511bbf70de9ef0666987c9cad449b54'

jviotti added a commit to jviotti/json-schema-website that referenced this pull request Nov 5, 2024
@jviotti
Copy link
Member Author

jviotti commented Nov 5, 2024

Just sent a PR upgrading community on main. Then we can rebase this one

benjagm pushed a commit that referenced this pull request Nov 8, 2024
@jviotti
Copy link
Member Author

jviotti commented Nov 8, 2024

@DarhkVoyd What about how? It should all be up to date

Copy link

github-actions bot commented Nov 8, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
website ✅ Ready (View Log) Visit Preview 31758ab

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Nov 9, 2024

Hey, I am just figuring out a problem with css styles missing in the preview/prod build. Then we should be good to go.

@benjagm
Copy link
Collaborator

benjagm commented Nov 23, 2024

@DarhkVoyd Is the CSS issue fixed? I love this section, but I am not sure about adding it as a top-level element in the menu.

I would feel more comfortable to add it as a card in the Community page or inside the overview section of the docs.

@DarhkVoyd
Copy link
Member

This CSS issue works fine in local development and both development and production builds, but it fails in the preview workflow.

I was unsure about placing “Pro-Help” as the top-level nav menu item, but I couldn’t think of a better place. Placing it elsewhere would reduce leads and visibility. Users should easily find out that it’s now possible. What do you think @jviotti ?

@jviotti
Copy link
Member Author

jviotti commented Nov 26, 2024

I don't have any strong opinions. Whatever we are fine with! Or we can also sort out linking to it in another PR

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.

4 participants