center the notifications vertically and horizontally #203
Conversation
| margin-top: 30px; | ||
| padding-left: 20px; | ||
| max-width: 800px; | ||
| width: 800px; |
There was a problem hiding this comment.
Was there any specific scenario/reason due to which you went ahead withwidth:800px
There was a problem hiding this comment.
After making it into flex, the notifications started taking 100% of width, did not want that. So I hardcoded the existing value of max-width and made it to width. Making this dynamic might not be a good idea in my opinion, because the content is limited, extending the notification holders while the text length remains same, might look a bit off. Let me know what you think :)
There was a problem hiding this comment.
Are we handling the same in different devices?
There was a problem hiding this comment.
Would we observe viewing issues in different screens for example 2k screen or a mobile
Is it visually tested for the same?
There was a problem hiding this comment.
Could we have a mobile view too please
| @@ -1,8 +1,12 @@ | |||
| .notifications-holder { | |||
| display: flex; | |||
There was a problem hiding this comment.
Why we even need flex? we have divs so anyhow they are going on new line.
There was a problem hiding this comment.
Flex is there to center the notifications
|
The issue is already addressed in #266. |
Fixes #202
I have also added slight margin between the orange element and the text
Before:
After:
