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(client): add tooltip for calendar-heatmap #36008

Merged
merged 9 commits into from Jun 11, 2019

Conversation

moT01
Copy link
Member

@moT01 moT01 commented May 10, 2019

This fixes up the heatmap calendar a little.

  • Add's the tooltip back
  • added react-tooltip package for the tooltip
  • rework the function to create the data for the map
  • 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.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

I'm hoping it can close these issues:
#35916
#17299
#17822
#22031

@moT01 moT01 requested a review from a team May 10, 2019 01:23
@thecodingaviator
Copy link
Contributor

I guess you'll need to remove package-lock.json ?

@moT01
Copy link
Member Author

moT01 commented May 13, 2019

I'm not sure - I thought it should be kept on when adding a package - did you get a chance to test it out @thecodingaviator?

@thecodingaviator
Copy link
Contributor

Yes, on doing npm i, I get this:

C:\Users\hp\Desktop\Web\freeCodeCamp>npm i

> @freecodecamp/freecodecamp@0.0.1 postinstall C:\Users\hp\Desktop\Web\freeCodeCamp
> npm run bootstrap


> @freecodecamp/freecodecamp@0.0.1 bootstrap C:\Users\hp\Desktop\Web\freeCodeCamp
> lerna bootstrap --ci

lerna notice cli v3.13.1
lerna info versioning independent
lerna info ci enabled
lerna info Bootstrapping 9 packages
lerna info Installing external dependencies
lerna ERR! npm ci exited 4294963248 in '@freecodecamp/client'
lerna ERR! npm ci stderr:
npm WARN prepare removing existing node_modules/ before installation
WARN tarball tarball data for react-ga@2.5.7 (sha512-UmATFaZpEQDO96KFjB5FRLcT6hFcwaxOmAJZnjrSiFN/msTqylq9G+z5Z8TYzN/dbamDTiWf92m6MnXXJkAivQ==) seems to be corrupted. Trying one more time.
npm ERR! path C:\Users\hp\Desktop\Web\freeCodeCamp\client\node_modules\react-ga\dist\react-ga.js
npm ERR! code EPERM
npm ERR! errno -4048
npm ERR! syscall unlink
npm ERR! Error: EPERM: operation not permitted, unlink 'C:\Users\hp\Desktop\Web\freeCodeCamp\client\node_modules\react-ga\dist\react-ga.js'
npm ERR!  { [Error: EPERM: operation not permitted, unlink 'C:\Users\hp\Desktop\Web\freeCodeCamp\client\node_modules\react-ga\dist\react-ga.js']
npm ERR!   cause:
npm ERR!    { Error: EPERM: operation not permitted, unlink 'C:\Users\hp\Desktop\Web\freeCodeCamp\client\node_modules\react-ga\dist\react-ga.js'
npm ERR!      type: 'OperationalError',
npm ERR!      '$error': '$error',
npm ERR!      cause:
npm ERR!       { errno: -4048,
npm ERR!         code: 'EPERM',
npm ERR!         syscall: 'unlink',
npm ERR!         path:
npm ERR!          'C:\\Users\\hp\\Desktop\\Web\\freeCodeCamp\\client\\node_modules\\react-ga\\dist\\react-ga.js' },
npm ERR!      isOperational: true,
npm ERR!      errno: -4048,
npm ERR!      code: 'EPERM',
npm ERR!      syscall: 'unlink',
npm ERR!      path:
npm ERR!       'C:\\Users\\hp\\Desktop\\Web\\freeCodeCamp\\client\\node_modules\\react-ga\\dist\\react-ga.js' },
npm ERR!   isOperational: true,
npm ERR!   stack:
npm ERR!    'Error: EPERM: operation not permitted, unlink \'C:\\Users\\hp\\Desktop\\Web\\freeCodeCamp\\client\\node_modules\\react-ga\\dist\\react-ga.js\'',
npm ERR!   type: 'OperationalError',
npm ERR!   '$error': '$error',
npm ERR!   errno: -4048,
npm ERR!   code: 'EPERM',
npm ERR!   syscall: 'unlink',
npm ERR!   path:
npm ERR!    'C:\\Users\\hp\\Desktop\\Web\\freeCodeCamp\\client\\node_modules\\react-ga\\dist\\react-ga.js' }
npm ERR!
npm ERR! The operation was rejected by your operating system.
npm ERR! It's possible that the file was already in use (by a text editor or antivirus),
npm ERR! or that you lack permissions to access it.
npm ERR!
npm ERR! If you believe this might be a permissions issue, please double-check the
npm ERR! permissions of the file and its containing directories, or try running
npm ERR! the command again as root/Administrator (though this is not recommended).

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\hp\AppData\Roaming\npm-cache\_logs\2019-05-14T08_06_08_395Z-debug.log

lerna ERR! npm ci exited 4294963248 in '@freecodecamp/client'
npm ERR! code ELIFECYCLE
npm ERR! errno 4294963248
npm ERR! @freecodecamp/freecodecamp@0.0.1 bootstrap: `lerna bootstrap --ci`
npm ERR! Exit status 4294963248
npm ERR!
npm ERR! Failed at the @freecodecamp/freecodecamp@0.0.1 bootstrap script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\hp\AppData\Roaming\npm-cache\_logs\2019-05-14T08_06_09_758Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 4294963248
npm ERR! @freecodecamp/freecodecamp@0.0.1 postinstall: `npm run bootstrap`
npm ERR! Exit status 4294963248
npm ERR!
npm ERR! Failed at the @freecodecamp/freecodecamp@0.0.1 postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\hp\AppData\Roaming\npm-cache\_logs\2019-05-14T08_06_09_975Z-debug.log

@moT01
Copy link
Member Author

moT01 commented May 14, 2019

so you're thinking that's from the package-lock file?

@ojeytonwilliams ojeytonwilliams self-requested a review May 14, 2019 12:30
@ojeytonwilliams
Copy link
Contributor

@moT01 I'm taking a look at this now. Is there an easy way to recreate those issues locally?

@thecodingaviator
Copy link
Contributor

so you're thinking that's from the package-lock file?

I'm not sure why else would it be happening, I'll try deleting the file and retrying once

@moT01
Copy link
Member Author

moT01 commented May 14, 2019

Honestly, no @ojeytonwilliams - I couldn't recreate most of the issues anywhere - so, I'm not sure what to do with it. I added the tooltip back in, which must have got removed at some point - and reworked the logic that creates the map to hopefully make it a little more robust - but I'm not sure if that was causing any problems in the first place.

@ojeytonwilliams
Copy link
Contributor

@moT01 Understood. I think the issues are all with a very old version of this, anyway, so some might have been fixed in the move to react-calendar-heatmap.

@moT01
Copy link
Member Author

moT01 commented May 14, 2019

To be a little bit more clear - when I added this tooltip in - the calendarValues variable only included data for dates which had activity from the user - and the tooltip would show like "null" for the items and date with no activity - so I added an empty object and filled it with all the dates starting from six months ago to give the tooltip some info for those dates. And the logic got reworked mostly because I needed that.

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.

This is a nice PR. While I ended up writing a lot of comments, they're mostly me worrying about edge cases that may never turn up. Overall I think this is a nice change.

Regarding the start of the calendar, I just want to stress that it's not something I'd hold up the merge for. I just wanted to see what you thought about it.

client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
client/src/components/profile/components/HeatMap.js Outdated Show resolved Hide resolved
@moT01
Copy link
Member Author

moT01 commented May 14, 2019

Yes, I like those suggestions @ojeytonwilliams - I obviously started at the beginning of six months ago because that's how it was, maybe there was a reason for that. But it seems to make more sense, to me anyway, doing it how you suggested - so I made those changes

@ojeytonwilliams
Copy link
Contributor

@moT01 Perhaps so - this looks nice though. Thanks for making those changes, it LGTM now.

@ahmadabdolsaheb Does everything look okay to you?

@thecodingaviator thecodingaviator self-requested a review May 14, 2019 16:00
@thecodingaviator thecodingaviator removed their request for review May 16, 2019 06:39
Copy link
Contributor

@ValeraS ValeraS left a comment

Choose a reason for hiding this comment

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

@moT01 if you agree with changes I will merge it.

Copy link
Member Author

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

Yes, I'm fine with these changes @ValeraS, but I had a thought... maybe we should change all the "item/items" to "point/points" - since that is how it's described above the heatmap (see picture) - also, I noticed that the "current streak" defaults to 1 - since it's right there in this file, maybe we should change that to default to zero in this PR.
Screen Shot 2019-05-26 at 6 34 51 PM

}
return {
'data-tip': `${valueCount}${format(value.date, 'MMMM Do, YYYY')}`
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm curious why you changed this @ValeraS?

@ValeraS
Copy link
Contributor

ValeraS commented Jun 4, 2019

Yes, I'm fine with these changes @ValeraS, but I had a thought... maybe we should change all the "item/items" to "point/points" - since that is how it's described above the heatmap (see picture) - also, I noticed that the "current streak" defaults to 1 - since it's right there in this file, maybe we should change that to default to zero in this PR.

Let's rename to point/points and change defaults for both streaks to 0.

@RandellDawson RandellDawson added the status: merge conflict To be applied to PR's that have a merge conflict and need updating label Jun 11, 2019
@ValeraS ValeraS changed the title fix/calendar-heatmap fix(client): add tooltip for calendar-heatmap Jun 11, 2019
@ValeraS ValeraS merged commit 397ff21 into freeCodeCamp:master Jun 11, 2019
@ValeraS ValeraS removed the status: merge conflict To be applied to PR's that have a merge conflict and need updating label Jun 11, 2019
@moT01 moT01 deleted the fix/calendar-heatmap branch June 11, 2019 13:57
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.

None yet

5 participants