Notifications Icon Circles #7830
Conversation
danjm
force-pushed
the
notifications-icon-circles
branch
from
bea152a
to
555b92e
Compare
Builds ready [3fd8a2f]
Page Load Metrics (690 ± 51 ms)
|
| } | ||
|
|
||
| &__icon { | ||
| z-index: 1; |
There was a problem hiding this comment.
Why is the z-index set on the icon? Also, why is it set both here and on line 9?
There was a problem hiding this comment.
The z-index is set so that the icon appears on top of the circle, which can have a coloured background (and defaults to a white background).
You are right that it does not need to be set in two places.
There was a problem hiding this comment.
We don't need to use z-index to ensure it appears on top though. It comes later in the DOM, so it'll be on top as long as it's a positioned element.
e.g.:
| z-index: 1; | |
| position: relative; |
I'd like to avoid the use of z-index if possible, as it can lead to unexpected results with multiple elements with z-index values overlap. If we ensure things are stacked according to the DOM order, everything "just works" the way you'd expect them to.
|
Do we have a use for all of these icons? We'll definitely use the error alert, but... for yellow, blue and green ones - I'm not sure we have any known use cases for blocking alerts that aren't errors 🤔 |
|
It seems that we do have use cases laid out for using these style of alerts for warnings and errors. Still not sure about the success and info cases though 🤔 Maybe we can leave those two out for now, until we find a use for them. |
…e iconSource required
danjm
force-pushed
the
notifications-icon-circles
branch
from
3fd8a2f
to
27939e3
Compare
Builds ready [27939e3]
Page Load Metrics (620 ± 62 ms)
|
| style={{ | ||
| height: size, | ||
| width: size, | ||
|
|
| } | ||
|
|
||
| &__icon { | ||
| z-index: 1; |
There was a problem hiding this comment.
We don't need to use z-index to ensure it appears on top though. It comes later in the DOM, so it'll be on top as long as it's a positioned element.
e.g.:
| z-index: 1; | |
| position: relative; |
I'd like to avoid the use of z-index if possible, as it can lead to unexpected results with multiple elements with z-index values overlap. If we ensure things are stacked according to the DOM order, everything "just works" the way you'd expect them to.
|
|
||
| export default class MessageCircleIcon extends Component { | ||
| static propTypes = { | ||
| type: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Nit: It might be helpful to be more specific here, so we're alerted if this component is used without a valid type.
| type: PropTypes.string.isRequired, | |
| type: PropTypes.oneOf(Object.keys(typeConfig)).isRequired, |
|
Where does the name "message-circle-icon" come from? Would "Alert Circle Icon" be a better name? As far as I know, these are only intended for use in alerts. |
Builds ready [47c627a]
Page Load Metrics (626 ± 41 ms)
|
This PR resolves a piece of #7389
I will be resolving that issue in parts. This first part only concerns adding the icons for toasting and notifications. It adds storybook stories for these to allow for design QA.
This PR depends on and is compared to the branch for #7688Edit: the piece of #7688 this depended on has been merged separately as #7884, so this now targets develop
Can be QA'd by checking out the branch and and running
yarn storybook.Screen shots:
