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

Allow Bar in Box #455

Closed
wants to merge 4 commits into from

Conversation

@bpierre
Copy link
Member

commented Aug 12, 2019

Builds on #454

bpierre added some commits Aug 12, 2019

@bpierre bpierre requested review from AquiGorka and sohkai Aug 12, 2019

@sohkai
Copy link
Member

left a comment

I like the changes to Box, and it makes sense to allow components to detect when they're in one.

But I had the impression that both Bar and Box were meant as primary surfaces, and to me it doesn't make much sense to put one in the other (and especially because it wouldn't work the other way around).

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

But I had the impression that both Bar and Box were meant as primary surfaces, and to me it doesn't make much sense to put one in the other (and especially because it wouldn't work the other way around).

Yes I agree it’s a bit weird… this is what it’s for:

image

Originally I wanted to pass <BackButton /> this way:

<Box
  heading={<BackButton />}
/>

But I wanted to avoid detecting its presence (using contains-component) only for that… but maybe it’s fine.

What about having a new prop, that would be mutually exclusive with heading?

<Box
  actions={<BackButton />}
/>
return useContext(getContext(name))
function useInside(name, withData) {
const { inside, data } = useContext(getContext(name))
return withData ? [inside, data] : inside

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Aug 13, 2019

Member

Could it happen that falsy withData wants to be passed down?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 13, 2019

Author Member

withData was only to request the data to be returned, but it has been removed 👉 #454 (comment)

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I don't know how common the pattern will be, but if it's not going to be that common (most apps would prefer to have separate Bars and Boxs?), we can also just apply the negative margin directly to the BackButton.

I just don't see this being common enough for us to design a case around it right now.

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I just don't see this being common enough for us to design a case around it right now.

It was kind of the idea: make this possible, nothing more. Or do you mean by not having any support for it in aragonUI? That could be an option too, yes.

bpierre added some commits Aug 13, 2019

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Closing as it can be done manually until it becomes a pattern.

@bpierre bpierre closed this Aug 13, 2019

@bpierre bpierre deleted the bar-in-box branch Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.