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(curriculum): forum leaderboard project #50241

Draft
wants to merge 89 commits into
base: main
Choose a base branch
from

Conversation

JoyShaheb
Copy link

Checklist:

@JoyShaheb JoyShaheb requested a review from a team as a code owner May 1, 2023 14:09
@github-actions github-actions bot added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. labels May 1, 2023
@codesee-maps
Copy link
Contributor

codesee-maps bot commented May 1, 2023

👀 Review this PR in a CodeSee Review Map

View the CodeSee Map of this change

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@JoyShaheb JoyShaheb marked this pull request as draft May 1, 2023 14:30
Copy link
Contributor

@jdwilkin4 jdwilkin4 left a comment

Choose a reason for hiding this comment

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

Left a couple of small comments 👍

Overall, I think this is a good start.
I think the thing to remember is that this project will be the last project campers do before the final certification project.

Some of your explanations can be shortened because campers will have already worked with those concepts in earlier projects and won't need as much explanation on how to do it. 👍

@bbsmooth
Copy link
Contributor

bbsmooth commented May 1, 2023

Just a few accessibility-related issues.

  • Is the empty column between Topics and Replies going to be used? If not then we should remove it.
  • It is acceptable for tables to have horizontal scroll if the page gets too narrow to show all the data nicely. I would add a min width to the table (preferably in rem) to keep it from getting too compressed.
  • I would also add a max-width to the table (preferably in rem) to keep the three smaller columns on the right from getting too far away from the Topics column.
  • Actually, you could do away with a max-width if you didn't set a width on the the table to begin with and just let it fill the space as needed. If you are worried about a topic getting too wide then you could se a max-width just on the first td.

@raisedadead raisedadead changed the title Feat: adding learn intermediate fetch promises and async await by building an fcc forum leaderboard feat(curriculum): forum leaderboard project May 7, 2023
@lasjorg
Copy link
Contributor

lasjorg commented May 11, 2023

We should check the response and try/catch the fetch.

We should build the string before adding it to the DOM, not concatenate it directly to the DOM. Another option would be to use map and join instead of forEach.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

I have reviewed steps 1 to 30 and left some feedback.
The challenges are simple and easy to understand. The tests also passed for each challenge. 🚀

Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

Hey Joy,

This is off to a good start. I've left some comments on the first 10 JavaScript steps, to help demonstrate some of the style and functional approaches we try to use.

Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

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

I wasn't able to review too many steps in detail, but hopefully these comments, along with the reviews Naomi, Jessica, and Zaira left, provide some guidance.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file was removed in a recent change. You can drop this change in favor of the suggestion in curriculum/challenges/_meta/learn-intermediate-fetch-promises-and-async-await-by-building-an-fcc-forum-leaderboard/meta.json.

You should see that this file has been removed when you rebase with upstream/main and solve the merge conflicts.

Copy link
Author

Choose a reason for hiding this comment

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

@scissorsneedfoodtoo need some here here buddy

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. We still need this file, but the format has changed and there are some other properties.

Here's an example of the current format of the file: https://github.com/freeCodeCamp/freeCodeCamp/blob/main/curriculum/challenges/_meta/learn-basic-javascript-by-building-a-role-playing-game/meta.json

client/i18n/locales/english/intro.json Outdated Show resolved Hide resolved
Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

I have reviewed the next 20 steps(31 to 50) and left some comments to simplify the instructions!👍

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Reviewed steps 51-60.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Reviewed steps 61-70.

There is a typo categorieList from steps 65 onwards. I have fixed this to categoryList and updated the tests/ seed code between 65-70. Is there a quick way to do it @JoyShaheb as this variable name is being used throughout the project?

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Some more comments for steps 71-80.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

With this comment, I have reviewed the entire project. 🚀 I have left some suggestions as well.

@JoyShaheb JoyShaheb requested a review from zairahira June 23, 2023 20:23
Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

@JoyShaheb the following steps are the HTML and CSS steps related to setting up the layout and styling and can be removed:

  • Steps #1 to #18(included)
  • Step #61
  • Steps #87 to #99(included)
  • Step #112
  • Step #114

@JoyShaheb JoyShaheb force-pushed the feat/adding-learn-intermediate-fetch-promises-and-async-await-by-building-an-fcc-forum-leaderboard branch from 238214d to bf7a692 Compare September 15, 2023 10:20
@github-actions github-actions bot removed the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front 3 2023 platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants