<Callout /> component #21273
<Callout /> component #21273
Conversation
teallarson
force-pushed
the
teal/free-ab-connectors-inline-enrollment-banner
branch
from
a48b2fa
to
d365f7a
Compare
teallarson
force-pushed
the
teal/free-ab-connectors-inline-enrollment-banner
branch
from
4825b29
to
2aa84ae
Compare
| [styles.blue]: variant === "blue", | ||
| }); | ||
|
|
||
| return <div className={containerStyles}>{children}</div>; |
There was a problem hiding this comment.
Take it or leave it: should we add a prop for an optional icon? I imagine a lot of callouts have an icon, and that would make it easy to ensure spacing between icon & text (or whatever is in {children}) is consistent.
There was a problem hiding this comment.
IMO no. I took out the existing prop for that as it was only used in 1-2 places.
There is also a spot where I plan on adding a button to a Callout, etc. Once I started considering different ways this might be used, I ended up deciding the important part here is really just the basic container.
I'm more in favor of keeping this very basic and unconcerned with what its children are personally. It does have flex, so it does space out its children consistently by default (which can be overridden by a classname if wanted)
There was a problem hiding this comment.
Having said that I just saw one more usage when I rebased.
However, with the move away from FA towards custom icons, we'd likely be passing in a ReactNode or a svg, at which point it feels to me like we may as well just accept it as a child... I could be swayed there though.
There was a problem hiding this comment.
Fine for me to skip this for now! I think when we do eventually move towards custom icons, we'll still want to enforce sizing constrains so that different <Callout> usages don't have different icon sizes, something like <Callout icon="exclamation-mark">{ ... some text or JSX ... }</Callout>. That would make it type safe and ensure alignments/size/spacing is always the same.
But no need to address that yet - we can probably enforce that later if/when we implement a custom Icon component: #20133
|
My reason for taking the semantic names out is that we don't have a consistent pattern for them app-wide currently. I don't feel strongly on it and agree it makes it easier to change theming later. Addressed in 657236c |
teallarson
deleted the
teal/free-ab-connectors-inline-enrollment-banner
branch
* genericize alert component * add className prop to alert * rename infobox "callout", swap in place for scss module * fix imports * use callout in place of alert * add blue variant, remove icon prop * remove font-size (this is already the default) * remove single use prop, use a classname instead * use semantic names, remove empty line * cleanup from rebase
What
Antecedent to some of our work in #21074 I've made some improvements (and a rename) to the artist formerly known as
<InfoBox />How
WarningBox) and make the icons children rather than propsRecommended reading order
InfoBox.*