feat(alert): add Header and Icon slots #1821
Conversation
lucivpav
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1821 +/- ##
==========================================
- Coverage 69.66% 69.64% -0.03%
==========================================
Files 874 874
Lines 7608 7612 +4
Branches 2243 2216 -27
==========================================
+ Hits 5300 5301 +1
- Misses 2300 2303 +3
Partials 8 8
Continue to review full report at Codecov.
|
…eact into feat/alert-header-and-icon
| bodyId?: string | ||
| } | ||
|
|
||
| export default AlertProps |
There was a problem hiding this comment.
Can you please clarify why we need to move out types?..
There was a problem hiding this comment.
Because both alertBehavior.ts and alertWarningBehavior.ts depends on it.
There was a problem hiding this comment.
Can we keep them alertBehavior.ts?
There was a problem hiding this comment.
I added it to alertWarningBehavior to avoid circular dependency (alertBehavior imports alertWarningBehavior)
packages/react/src/lib/accessibility/Behaviors/Alert/alertWarningBehavior.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Alert/alertStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/lib/accessibility/Behaviors/Alert/alertProps.ts
Outdated
Show resolved
Hide resolved
| ...accessibility.attributes.content, | ||
| className: Alert.slotClassNames.icon, | ||
| styles: styles.icon, | ||
| xSpacing: 'after', |
There was a problem hiding this comment.
| xSpacing: 'after', |
We can move this to styles instead?
There was a problem hiding this comment.
But xSpacing is a prop of Icon. If I set it in styles prop, the value is not taken into account.
There was a problem hiding this comment.
Problem for styles
It's just a margin value. We should avoid styling via props as it created hidden dependencies. We have already discussed this problem and it's related to children API.
<Alert icon="book" />
// This things look equal 🤔
// But rendered result will be different as there are no matching styles for it
<Alert>
<Icon name="book" />
</Alert>
// With usage component props we are making it even more complicatedTo coupled
If horizontalSpace variable in IconVariables will be changed, it will accidentally affect this component. Will be it valid?
| <> | ||
| {Box.create(header, { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Changed it to Text
…eact into feat/alert-header-and-icon
This PR adds an ability to specify
headerandiconprops when using anAlertcomponent.Example: