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(calendar): remove recompose and convert to hooks #1040

Merged
merged 19 commits into from
Aug 2, 2020

Conversation

diegosps
Copy link
Contributor

Hi!
I removed the dependency from recompose and ported the code to use hooks. This way, Calendar should be free from #884
I tried my best to follow the commit pointed by @wyze on that issue, but I have no experience with Typescript. Please, double check packages/calendar/index.d.ts (and everything else, to be honest)

I also changed:

1 - I think the legendFormat might be better suited at the legend component, but since I was following some pattern with useValueFormatter, I wasn't able to put it there. Maybe I need a little more thinking.

2 - For each day, the SVG Calendar rendered two rectangles: one with the spacing and the color fill, one with transparent fill and mouse events, but without any spacing. The point here was to be able to raise events while the mouse was over the "empty" space created by daySpacing.
After migrating to hooks, I was experiencing a bug where, sometimes, the tooltip didn't show. After I eliminated the second rect and put the mouse events on the first one, this bug disappeared. Also, during my limited visual tests, the tooltip fluidity was way better this way. The 'downside' of this change is: we are no longer able to see tooltips while hovering the empty space created by daySpacing. I'm ok with that.

Other possible break change is: tooltipFormat is gone.

I also created some stories for the above changes and for the custom color scale function.

I'm not sure if I need to change anything for website and api.

Please, feel free to tell me what I can do to improve this patch.

Thanks

@wyze
Copy link
Contributor

wyze commented Jul 31, 2020

@diegosps Can you get the build passing please? Looks like it's just a formatting issue.

@diegosps
Copy link
Contributor Author

All passing now

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

I need to review the code still. Looks like the custom tooltip story for the non-canvas version no longer works, it is just a black box.

As far as website and api changes, most of the external API remained the same so I don't think we need changes there. At least not the website, unsure about API really.

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Mostly minor things.

packages/calendar/src/Calendar.js Outdated Show resolved Hide resolved
packages/calendar/src/Calendar.js Outdated Show resolved Hide resolved
packages/calendar/src/Calendar.js Outdated Show resolved Hide resolved
packages/calendar/src/Calendar.js Outdated Show resolved Hide resolved
packages/calendar/src/Calendar.js Outdated Show resolved Hide resolved
packages/calendar/src/CalendarCanvas.js Outdated Show resolved Hide resolved
packages/calendar/src/CalendarCanvas.js Outdated Show resolved Hide resolved
packages/calendar/src/CalendarCanvas.js Outdated Show resolved Hide resolved
packages/calendar/src/compute.js Outdated Show resolved Hide resolved
packages/calendar/src/hooks.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@diegosps diegosps left a comment

Choose a reason for hiding this comment

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

I'm not sure what I'm supposed to do when submitting these review changes.
I will push a new batch of commits and this as a comment.
I was also not sure if I should be the one marking the things as resolved.
I left the return part unresolved for you to see.
Thanks for the feedback

@wyze
Copy link
Contributor

wyze commented Aug 1, 2020

Yeah just resolve the comments as you make the changes and push! :)

@diegosps diegosps requested a review from wyze August 1, 2020 23:25
Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the improvements!

@wyze wyze changed the title Remove recompose from Calendar and other changes feat(calendar): remove recompose and convert to hooks Aug 2, 2020
@wyze wyze merged commit daebd61 into plouc:master Aug 2, 2020
@czonios
Copy link

czonios commented Sep 19, 2020

Hello everyone,

any word on when the release containing this change will be released to npm? I specifically need the granularity feature, to show only a few months.

Keep up the great work, love this library!

@wyze
Copy link
Contributor

wyze commented Sep 19, 2020

Unfortunately I cannot make releases to npm yet, so we are waiting on @plouc to get a release out.

@mthanasi
Copy link

Hey guys,

Firstly, awesome work!
I really need the granularity feature, to show only a 1-2 months max. Any update on when will this change be released to npm?

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

4 participants