-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: elements displayed in block body #922
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSS is magic!
src/components/message/styles.scss
Outdated
display: inherit; | ||
flex-direction: column; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Looks like 'block-body' contains a number of different elements. Did we verify rendering looks correct for all of the cases where other things are rendered and not just the message?
- Can we remove the flex-direction and gap? Those only apply if you are display: flex, I think.
- Can we just remove
display
completely here? Why does it need to inherit from the parent? I think that'd make it difficult to figure out what it's actually rendering as.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Dale!
-
I went through and checked the elements and all looked okay to me. I compared with development. From what I could see the elements render okay. Is there anything that comes to mind that would potentially be broken in the UI ? I just had another quick check and appears okay.
-
Good shout yeah I can remove those.
-
Yeah, same as above, I should be able to just remove it here. Will make that change. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was partly why I didn't want to merge straight away just in case I may have missed something, felt like an update that was 'too easy' to not have caused any unwanted changes in the ui 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything that comes to mind that would potentially be broken in the UI ?
I guess mainly the gap that was there. I imagine that was probably the reason for it being flex in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the above. Removing flex wouldn't have been a good solution here. Instead, wrapping the message content (text and links) fixes the issue without having to touch any its parent containers, so the message block positioning/element spacing won't be affected. This should be a cleaner solution. Thanks!
What does this do?
Why are we making this change?
How do I test this?
Key decisions and Risk Assessment:
Things to consider:
BEFORE:
AFTER: