[WIP] Add Banner component #112
Conversation
| } | ||
|
|
||
| actionBanner :: Array JSX -> PropsModifier BannerProps | ||
| actionBanner actions f = |
There was a problem hiding this comment.
I wonder if this would be better as a LumiComponent instead of a PropsModifier.
There was a problem hiding this comment.
This does seem like a strange way to use it.. hmm
There was a problem hiding this comment.
Yeah, I think so too. Would you prefer having it as a LumiComponent?
| data Banner = Banner | ||
|
|
||
| type BannerProps = | ||
| ( component :: Banner |
There was a problem hiding this comment.
We don't export the single constructor of the Banner data type so a banner can be identified in a PropsModifier as forall props. PropsModifier (component :: Banner | props).
There was a problem hiding this comment.
This is an interesting idea, but I'm not sure it's really necessary.
There was a problem hiding this comment.
It is necessary if actionBanner is a LumiComponent, so that we can apply all of the modifiers that work on banner on it too.
There was a problem hiding this comment.
I'm not sure I follow.. wouldn't it work on any component with the same prop shape?
There was a problem hiding this comment.
Yes, but actionBanner wouldn't have the same prop shape as banner if it were a LumiComponent:
type BannerProps =
( dismissable :: Boolean
, content :: Array JSX
)
type ActionBannerProps =
( dismissable :: Boolean
, actions :: Array JSX
, content :: Array JSX
)
| data Banner = Banner | ||
|
|
||
| type BannerProps = | ||
| ( component :: Banner |
There was a problem hiding this comment.
This is an interesting idea, but I'm not sure it's really necessary.
| } | ||
|
|
||
| actionBanner :: Array JSX -> PropsModifier BannerProps | ||
| actionBanner actions f = |
There was a problem hiding this comment.
This does seem like a strange way to use it.. hmm
| , desktopQuery $ css | ||
| { padding: str "12px 24px" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Nice, this api really shines here
src/Lumi/Styles/Responsive.purs
Outdated
| { "@media (min-width: 860px)": nested style | ||
| } | ||
|
|
||
| onDesktop :: forall props. PropsModifier () -> PropsModifier props |
There was a problem hiding this comment.
Should props be passed to both of these types, or props1 and props2? The empty row seems restrictive.
There was a problem hiding this comment.
We can't have an open row here, otherwise we wouldn't be able to apply the argument to the props set, because we don't know what it is. It's not restrictive given that this should only work for styles anyway, and style modifiers are always forall props. PropsModifier props.
arthurxavierx
force-pushed
the
arthur/banner
branch
from
f96fc10 to
f2bf70e
Compare
|
Closing this since the work is being done in #143 now |
No description provided.