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(map): Removed NA text for challenge without time estimat #16479

Merged
merged 1 commit into from Jan 24, 2018
Merged

fix(map): Removed NA text for challenge without time estimat #16479

merged 1 commit into from Jan 24, 2018

Conversation

lgrzybowski
Copy link
Contributor

@lgrzybowski lgrzybowski commented Jan 12, 2018

Closes #16454

Pre-Submission Checklist

  • Your pull request targets the staging branch of freeCodeCamp.
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/signin-issue)
  • You have only one commit (if not, squash them into one commit).
  • All new and existing tests pass the command npm test. Use git commit --amend to amend any fixes.

Type of Change

  • Small bug fix (non-breaking change which fixes an issue)

Checklist:

Closes #16454

Description

I had run already implemented tests, but I think for react project it would be really nice to use jest with snapshot testing. One more idea would be to add eslint check as part of npm test command

@BerkeleyTrue BerkeleyTrue added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Jan 12, 2018
@vkWeb
Copy link
Member

vkWeb commented Jan 12, 2018

@lgrzybowski Next time please comment on the issue you're working on so that the other participants know that you're on it.

@ghost
Copy link

ghost commented Jan 12, 2018

The if/else block added in Block.jsx produces a lot of duplicate code. Perhaps there's a more elegant way to inject the time span.

{
  time
    ? <span className={ `${ns}-block-time` }>({ time })</span>
    : ''
}

@r1chard5mith
Copy link

Or you could just leave the time as N/A and test for it in the JSX and then it's a one line change to one file only.

<span className={ ${ns}-block-time}>{ time === "N/A" ? "" : "(" + time + ")" }</span>

@camperbot
Copy link
Contributor

@lgrzybowski updated the pull request.

@lgrzybowski
Copy link
Contributor Author

@0x0936 This should looks better now.

{
time
? <span className={ `${ns}-block-time` }>({ time })</span>
: ''

This comment was marked as off-topic.

@raisedadead raisedadead added status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Jan 18, 2018
@camperbot
Copy link
Contributor

@lgrzybowski updated the pull request.

@lgrzybowski
Copy link
Contributor Author

@BerkeleyTrue Thx for your review this should be looking better now.

@BerkeleyTrue BerkeleyTrue merged commit 20a3363 into freeCodeCamp:staging Jan 24, 2018
@BerkeleyTrue
Copy link
Contributor

@lgrzybowski nice work!

Happy Coding

@QuincyLarson
Copy link
Contributor

@lgrzybowski Awesome! Thank you for your contribution to the codebase!

@lgrzybowski lgrzybowski deleted the fix/change_NA_to_blank_when_no_estimation branch January 24, 2018 07:46
ValeraS pushed a commit to ValeraS/freeCodeCamp that referenced this pull request Oct 12, 2018
…to_blank_when_no_estimation

fix(map): Removed NA text for challenge without time estimat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

7 participants