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

[WIP] Major API changes #179

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

declan-warn
Copy link
Contributor

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.

@changeset-bot
Copy link

changeset-bot bot commented Mar 25, 2021

⚠️ No Changeset found

Latest commit: e51a5f6

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
Copy link

@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.

<PropsTable>
  {(props) => do whatever}
</PropsTable>

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 :)

@declan-warn
Copy link
Contributor Author

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.

@davidfloegel
Copy link

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 🙂

@danieldelcore
Copy link
Contributor

Amazing!

Sorry I'm still gathering context on this!
I'm liking this idea more and more after catching up with Declan this afternoon 😄
I think creating a generic "Layout" component (there's probably a much better name for this haha) in addition to the existing layout components is the way to go.

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!!

@davidfloegel
Copy link

davidfloegel commented Apr 8, 2021

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?
usePropsExplorer(Component)

I'm only on my phone atm but I will create a draft PR if that is ok with everyone :)

@davidfloegel
Copy link

@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 :)

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

3 participants