-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
- Add bottom margin to <p> tags in comment previews - <p> -> <div> for preview wrapper - change spout from a separate div to a ::before pseudo-elem Co-authored-by: Max Duval <[email protected]>
Co-authored-by: Max Duval <[email protected]>
Size Change: +219 B (0%) Total Size: 218 kB
|
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.
LGTM - just wondering where the padding and width figures come from?
The ones in the story components? I took them from the stories for CommentBlockComponents, where there was already the padding, and then Oliver suggested adding a width: https://github.com/guardian/dotcom-rendering/pull/4838/files#r866602666 I thought these seemed fairly appropriate here too, but to be honest I didn't give it too much thought, so happy to adapt if you think it suitable 👍 |
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.
Nice 👍
Thanks! I'll merge now. @oliverlloyd, I don't think I have the required permissions to publish the package to npm. I'm not sure how often we release updates to the package, but I'd be interested in going through the publishing process with someone next time it's released. |
What does this change?
It adds bottom margin to paragraph tags.
Why?
This makes the styling in comment previews more consistent with the styling of submitted comments. This should address the first part of this issue.
Screenshots
(From storybook)
Previously, the storybook rendering of comment previews didn't match the production site, because the dotcom site applies reset styles which weren't being applied in storybook. This PR adds the reset styles (globally, merging #572) and also adds spacing to the preview comments. So there shouldn't be much visual change in the storybook itself, but the storybook should now match dotcom.