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

Ordinal network frame conversion #616

Merged
merged 5 commits into from
Jan 26, 2022
Merged

Conversation

alexeyraspopov
Copy link
Member

Converting some more stuff to functions. In the same way as XYFrame, Ordinal and Network frames were pretty easy to move to function components, by using previously implemented useDerivedStateFromProps and useLegacyUnmountCallback. I used the old docs and examples to test the changes, at least to ensure I'm not breaking anything. Couple of small details regarding this PR:

  • In NetworkFrame there were some unused props (like onNodeOut) that I removed from both the component and related typings
  • In OrdinalFrame there were possibly incorrect default props values for oScaleType: scaleBand and rScaleType: scaleLinear which TS pointed to. So I changed the values to oScaleType: scaleBand() and rScaleType: scaleLinear() (note the parentheses) and the type error's gone.

After this PR is merged, only two components are left to be converted: InteractionLayer and VisualizationLayer. Those are slightly harder to work with. I think I'll be able to finish my work on #601 first and then continue with the layers conversion.

@alexeyraspopov alexeyraspopov requested a review from emeeks January 26, 2022 01:58
@vercel
Copy link

vercel bot commented Jan 26, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nteract/semiotic/34QYqP8YALsjtnvBD7xBcFviHjfL
✅ Preview: https://semiotic-git-ordinalnetworkframeconversion-nteract.vercel.app

[Deployment for ec02017 failed]

OrdinalFrame's `scaleType` property takes the actual D3 function and not the scale it returns.
@emeeks
Copy link
Member

emeeks commented Jan 26, 2022

There was only one aspect that I had to muddle through to fix: we want people to easily be able to change the scale types for the numerical scales, as such oScaleType is sort of the odd man out. We really don't want to let people change that but, then again, it might be a way for them to hack what they want into OrdinalFrame. But to get this to work we need to either have some pretty sophisticated typing for those props or just rely on using any and leave developers a bit out in the cold when using them.

We know that Semiotic users want to replace the default numerical scales with scalePow and scaleLog etc and also polylinear scales (scaleLinear with more than two steps). The current setup allows them to send either scalePow for example or scalePow(2) and still get results. To deal with this, though, we have to have some way of knowing if the scale is instantiated or not. My typescript fu isn't strong enough for that, so you can see I cast them as any which we can go back to and clean up, but I wanted to capture the reasoning here.

That said I could only get serve-examples to work, but it seems like the OrdinalFrame functionality there is in order, so I'm going to approve and merge the PR and when I can test it more robustly, if I see any issues, I'll tackle those.

@emeeks emeeks merged commit 8b88c08 into main Jan 26, 2022
@alexeyraspopov alexeyraspopov deleted the ordinalNetworkFrameConversion branch January 26, 2022 20:23
@emeeks emeeks restored the ordinalNetworkFrameConversion branch January 27, 2022 02:59
@emeeks emeeks deleted the ordinalNetworkFrameConversion branch January 27, 2022 02:59
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.

2 participants