-
Notifications
You must be signed in to change notification settings - Fork 28
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
[WIP] Major API changes #179
base: master
Are you sure you want to change the base?
Conversation
|
@declan-warn hi! Outsider here 😬 I like the idea of this PR because we want to have a pretty custom layout for our props table. I was wondering what your thoughts are on using render props rather than passing in a layout - which is still quite restricting. I.e.
For example, we want to be able to group and sort our props and have collapsable sections which we wouldn't be able to do using just the custom layout component. Also I believe generally it would be less maintenance for you guys if you provided one option to do custom things rather than supporting x different versions of a row. Just food for thought! Let me know what you think :) |
Hi @davidfloegel, Thanks for the input! I really like the idea of using render props instead. Being able to sort and group props is not something I've considered, but it seems completely reasonable. I'll have a think on what this would look like and bring it up with @danieldelcore. |
Hey @declan-warn, amazing! I'd be super keen to make a PR for this - I've always wanted to contribute to something open source and this I could actually do 😆 Let me know what you guys decide 🙂 |
Amazing! Sorry I'm still gathering context on this! Something like this: import { Layout, Prop } from 'pretty-proptypes';
<Layout>
{(props) => {
return (
<div className="my-custom-layout">
${props.map(prop => <Prop {...prop} />)}
</div>
);
}
</Layout> I would really welcome a PR from you @davidfloegel 💯 I think this is a great opportunity to contribute 😄 (Hope that's ok @declan-warn) Cheers, guys!! |
Not sure whether "Layout" is the right name, as all it does would be providing the props itself right? (edit: just seen you said this yourself haha) If you guys want to keep the default layout that you use at atlassian and not make it headless (which probably makes sense), then maybe we could have a separate component for that. For me, PropsProvider or PropsExplorer which only takes the component you want the props for. Then you could keep your existing code as is and instead wrap that into that new provider. Alternatively, how do you guys feel about a hook? I'm only on my phone atm but I will create a draft PR if that is ok with everyone :) |
@danieldelcore @declan-warn I've had some time before work, so I've gone ahead and created a draft PR. I can't add reviewers for some reason but please do have a look and add yourself :) |
Seems to work fairly well. Shifted a bit from the original idea.
Needs a little bit of thought about wrappers. E.g. the table layout now requires the user to provide the wrapping table.
Needs docs, etc.