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

PoC: Create new usePropsExplorer hook #182

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

PoC: Create new usePropsExplorer hook #182

wants to merge 3 commits into from

Conversation

davidfloegel
Copy link

Summary

This is a POC for introducing a usePropsExplorer hook to give consumers the option to create a fully customised props table.

Please note that this is a work in progress - I've only done the changes required to make this work. If this is an approach you'd like to take forward, I will continue this PR!

Do let me know what you think!

Why is this required?

The current library is great and works like a charm, however, there are use-cases where the provided layouts are too restricting.

At your company, we're building a docs application for our design system and want to be able to sort and group our props into collapsable sections. For example, we want to take all the "aria" attributes from a button and move them into a specific "accessibility" section, all onX events into an "event handler" section etc.

In order to do so, we need a list of all the props that we can then manipulate and display in any way we want.

Currently, this is not possible as there are only three templates to choose from to display the props.

There is a draft PR open (#179) which almost addresses this issue but being able to pass in a "Custom Layout", however, I see this causing issues down the line where you'll have to make sure that all existing layouts still work plus custom layouts are honoured and so on.

This "headless" approach has worked very well for me personally in the past so I thought I'd create a PR showing you this.

This PR could also solve issues such as #141 , #153 and #178 because you could simply tell the consumer "use the hook, build your own thing". Your library would stay highly maintainable, minimal with reasonable defaults.

Implementation Details

I have created a usePropsExplorer hook within the pretty-proptypes package that returns an array of the prop types (or null). The code is pretty much re-used from the PropsTable implementation.

I've created a new storybook story that uses the new hook to display a custom table as example.

Todo

Should this PR be a desired approach, things left to do include:

  • Refactor the code written and add types using flow
  • Write tests for the new hook
  • Write documentation for the new hook

Thoughts

Because the hook will be providing all the props, the PropsTable implementation could be reduced to use the new hook created. That way it would save code duplication.

@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2021

⚠️ No Changeset found

Latest commit: 2904bbc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@davidfloegel davidfloegel marked this pull request as draft April 8, 2021 08:07
@atlassian-cla-bot
Copy link

atlassian-cla-bot bot commented Apr 8, 2021

Hooray! All contributors have signed the CLA.

@davidfloegel
Copy link
Author

@danieldelcore @declan-warn hey guys, any thoughts on this? 🙂

@danieldelcore
Copy link
Contributor

Hey @davidfloegel 👋

So sorry about the delay, been a busy week for us this week 😅!

Really love the idea, but I'm still a little bit confused about the difference between using this hook vs just using the type object directly?

So instead of:

import TypeScriptComponent from './TypeScriptComponent';

const propTypes = usePropsExplorer({ component: TypeScriptComponent });

Could we just do:

import TypeScriptComponent from './TypeScriptComponent';

const propTypes = TypeScriptComponent;

Is the use of getPropTypes that is missing here?

Maybe just doing this will be enough for your use-case?

import { getPropTypes } from 'pretty-proptypes';
import TypeScriptComponent from './TypeScriptComponent';

const propTypes = getPropTypes(TypeScriptComponent);

return null;
};

export default function usePropsExplorer(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @davidfloegel: Love your work ❤️. Sorry for jumping like this. One question around naming here. Not sure if we really call this a hook or a util? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Hey dude! Thanks :)

Yeah, I did like the idea of this being a hook but in the end I think it doesn't have to be.

Daniel had a good point though about just exposing the getPropTypes function - so maybe that's more than enough

Comment on lines +12 to +28
if (component) {
/* $FlowFixMe the component prop is typed as a component because
that's what people pass to Props and the ___types property shouldn't
exist in the components types so we're just going to ignore this error */
if (component.___types) {
props = { type: 'program', component: component.___types };
} else {
/* eslint-disable-next-line no-console */
console.error(
'A component was passed to <Props> but it does not have types attached.\n' +
'babel-plugin-extract-react-types may not be correctly installed.\n' +
'<Props> will fallback to the props prop to display types.'
);
}
}

let propTypes = getProps(props) || null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice man ❤️. BTW is it possible to simplify this with optional chaining and probably we can return earlier if everything works and print error at last & return null then? 🤔

@davidfloegel
Copy link
Author

Hey @davidfloegel 👋

So sorry about the delay, been a busy week for us this week 😅!

Really love the idea, but I'm still a little bit confused about the difference between using this hook vs just using the type object directly?

So instead of:

import TypeScriptComponent from './TypeScriptComponent';



const propTypes = usePropsExplorer({ component: TypeScriptComponent });

Could we just do:

import TypeScriptComponent from './TypeScriptComponent';



const propTypes = TypeScriptComponent;

Is the use of getPropTypes that is missing here?

Maybe just doing this will be enough for your use-case?

import { getPropTypes } from 'pretty-proptypes';

import TypeScriptComponent from './TypeScriptComponent';



const propTypes = getPropTypes(TypeScriptComponent);

Ha no problem! I know how it is 😬

Now that is actually a very good point. I think for me that would be enough, but from what I can see there are some modifications made for flow components, so just exposing getPropTypes would only be a short-term hack. Unless we move that logic into the getPropTypes function?

But then, yes, that'd be the same 😆

@davidfloegel
Copy link
Author

Closed in favour of #206

@davidfloegel
Copy link
Author

@danieldelcore so I've just tried the above PR locally but when I try to use getPropTypes exported from pretty-proptypes it returns an undefined.

So not sure what's going on but would love some help on this as I'm picking up our docs app again and we need a props table 🤞

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.

3 participants