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

fix(learn): remove cta and add current challenge button #38807

Merged
merged 6 commits into from May 21, 2020
Merged

fix(learn): remove cta and add current challenge button #38807

merged 6 commits into from May 21, 2020

Conversation

Twaha-Rahman
Copy link
Contributor

@Twaha-Rahman Twaha-Rahman commented May 13, 2020

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

ref #38781

Removed "If you are new to coding, we recommend you start at the beginning." message if the user has completed more than 15 challenges.

@Twaha-Rahman Twaha-Rahman requested a review from a team May 13, 2020 00:29
@gitpod-io
Copy link

gitpod-io bot commented May 13, 2020

@Twaha-Rahman
Copy link
Contributor Author

If any additional changes are required, please let me know!

@Twaha-Rahman
Copy link
Contributor Author

@ahmadabdolsaheb I think this PR is ready for a round of review😀

@raisedadead
Copy link
Member

Hi @Twaha-Rahman thanks for your PR.

Quick Note: All changes are reviewed in due course. Thanks for your patience.

@raisedadead raisedadead 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. labels May 13, 2020
@raisedadead
Copy link
Member

Closing and re-opening to Trigger CI build stall.

@raisedadead raisedadead reopened this May 13, 2020
@gitpod-io
Copy link

gitpod-io bot commented May 13, 2020

</h4>
</Col>
) : (
''
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic wants to be

if completedChallenge < threshold then show old text
else
show link to the current challenge

we could use https://github.com/freeCodeCamp/freeCodeCamp/blob/master/client/src/components/helpers/CurrentChallengeLink.js, with some style tweaks, to handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the issue #38781 @ahmadabdolsaheb stated this.

He wanted these two as a separate feature if I am not misunderstanding....😅

He said that-

remove "If you are new to coding, we recommend you start at the beginning." message if the user has completed a certain number of challenges.

So, should I update the PR or is this enough @ojeytonwilliams ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed that discussion. It looks to me like the final decision hasn't been made yet. Given that, please don't implement what I've suggested until that decision's been clarified, as it may not be what's wanted.

Copy link
Member

Choose a reason for hiding this comment

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

Logic states that:

For a signed in user:

  1. The start here action should be when a user has less completed challenges than a certain threshold no. of challenges.

  2. There should be a go to a current challenge action (a button in the stack is a good idea)
    a. Should be shown only on signed in
    b. Only when a user has completed at-least one challenge.

  3. The sign in button is obviously not there in this case.

For a non-signed in user:

  1. The start here action should be there always, because we do not save progress.
  2. The current challenge action should not be there, because we would want them to click the "sign in and save your progress" action instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

The stack currently being "View my Portfolio", "Update my account settings"?.

Copy link
Member

Choose a reason for hiding this comment

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

The stack currently being "View my Portfolio", "Update my account settings"?.

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for clarifying guys! Much appreciated!

@raisedadead raisedadead added the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label May 19, 2020
@Twaha-Rahman
Copy link
Contributor Author

Twaha-Rahman commented May 21, 2020

@ojeytonwilliams @raisedadead I've updated the PR according to the suggestions made -

  1. For a signed in user:
  • The start here action is shown when a user has less completed challenges than a certain threshold no. of challenges. (I have selected 15 challenges as the threshold for now)

  • There is a go to a current challengebutton in the stack-
    a. Is shown only to signed in users
    b. User has to completed at-least one challenge to see this button

  1. For a non-signed in user:
  • The start here action is always there
  • The current challenge action isn't there

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

When reviewing I noticed that completedChallengeCount only updates when a new user object is obtained (i.e. on page reload).

I don't think this needs fixing, since the idea is to guide people who have been away and come back, but it's a small gotcha when QAing.

client/src/components/Intro/index.js Outdated Show resolved Hide resolved
client/src/components/Intro/index.js Show resolved Hide resolved
client/src/components/helpers/CurrentChallengeLink.js Outdated Show resolved Hide resolved
@Twaha-Rahman
Copy link
Contributor Author

Twaha-Rahman commented May 21, 2020

@ojeytonwilliams I've fixed the problems. Ready for another round of review😁

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

@Twaha-Rahman thanks for making those changes. It LGTM now!

@Twaha-Rahman
Copy link
Contributor Author

Glad I could help!

@raisedadead raisedadead changed the title removed "If you are new to coding, we..." message for old learners fix(learn): remove cta and add current challenge button May 21, 2020
@raisedadead raisedadead merged commit d01ce3b into freeCodeCamp:master May 21, 2020
@Twaha-Rahman Twaha-Rahman deleted the improvements-to-learn branch May 21, 2020 13:06
abbathaw pushed a commit to abbathaw/freeCodeCamp that referenced this pull request Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants