ui: remove dead css and refactor markdown css #1290
Conversation
There was a problem hiding this comment.
Comparing with dev, it looks like we're losing some styling on the code snippets. I verified that headers are rendering properly. Tables look weird, but they look weird in Prod/Dev too so that can be a separate issue w/ an isolated fix. Can we avoid using this CSS altogether in favor of something more maintainable?
Dev as of today:
PR Screenshot:
|
Ah good catch, I didn't think to look there. I wonder what's getting in the way there 🤔. This looks like the most promising alternative: https://github.com/react-syntax-highlighter/react-syntax-highlighter |
|
@Forfold after reviewing, I don't think that library suits our needs. I'm quite happy with this as is, but let me know if you have any other suggestions |
web/src/app/util/Markdown.js
Outdated
| '& h1, h2, h3, h4, h5, h6, p': { | ||
| marginTop: 0, | ||
| marginBottom: '0.5rem', | ||
| }, | ||
| '& hr': { | ||
| margin: '1.5rem 0', | ||
| }, | ||
| '& th': { | ||
| textAlign: 'left', | ||
| paddingRight: '8px', | ||
| }, | ||
| '& td': { | ||
| paddingRight: '8px', | ||
| }, | ||
| '& pre': { | ||
| display: 'block', | ||
| padding: '9.5px', | ||
| margin: '0 0 10px', | ||
| fontSize: '13px', | ||
| lineHeight: '1.42857143', | ||
| color: '#333', | ||
| wordBreak: 'break-all', | ||
| wordWrap: 'break-word', | ||
| backgroundColor: '#f5f5f5', | ||
| border: '1px solid #ccc', | ||
| borderRadius: '4px', | ||
| }, |
There was a problem hiding this comment.
Everything is rendering as expected now, though imho I think we should be using the defaults from this library instead of this here and just focus on getting the code block elements rendered with the proper colors. To be blunt, I'm not really in favor of maintaining things like lineHeight: '1.42857143' if we can avoid it
There was a problem hiding this comment.
Doing some functional testing to see the differences, outside of the code elements I'm only seeing the CSS here compact the lines together.
There was a problem hiding this comment.
I'm guessing that lineheight was ripped from some computed source a while back along with the fontsize and padding, maybe others. Probably best to open an issue for touching up all markdown styles so we don't bikeshed on this too much
There was a problem hiding this comment.
Since this PR is for removing the dead CSS but also refactoring, I'd be fine with making those changes here. We should be able to do away with everything except the color bits for pre/code and be in a good state. I went ahead and double-checked what we'll need to keep (also adjusting one px value to rem). Will comment with the commit suggestion
There was a problem hiding this comment.
I did some further digging, and this code is a remnant from when we were using a completely different markdown library altogether (way back at the beginning of 2018)- so I definitely think we're good here to remove
web/src/app/util/Markdown.js
Outdated
| markdown: { | ||
| '& h1, h2, h3, h4, h5, h6, p': { | ||
| marginTop: 0, | ||
| marginBottom: '0.5rem', | ||
| }, | ||
| '& hr': { | ||
| margin: '1.5rem 0', | ||
| }, | ||
| '& th': { | ||
| textAlign: 'left', | ||
| paddingRight: '8px', | ||
| }, | ||
| '& td': { | ||
| paddingRight: '8px', | ||
| }, | ||
| '& pre': { | ||
| display: 'block', | ||
| padding: '9.5px', | ||
| margin: '0 0 10px', | ||
| fontSize: '13px', | ||
| lineHeight: '1.42857143', | ||
| color: '#333', | ||
| wordBreak: 'break-all', | ||
| wordWrap: 'break-word', | ||
| backgroundColor: '#f5f5f5', | ||
| border: '1px solid #ccc', | ||
| borderRadius: '4px', | ||
| }, | ||
| '& code': { | ||
| padding: '2px 4px', | ||
| fontSize: '90%', | ||
| color: '#c7254e', | ||
| backgroundColor: '#f9f2f4', | ||
| borderRadius: '4px', | ||
| }, | ||
| '& pre code': { | ||
| padding: 0, | ||
| fontSize: 'inherit', | ||
| color: 'inherit', | ||
| whiteSpace: 'pre-wrap', | ||
| backgroundColor: 'inherit', | ||
| borderRadius: 0, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
| markdown: { | |
| '& h1, h2, h3, h4, h5, h6, p': { | |
| marginTop: 0, | |
| marginBottom: '0.5rem', | |
| }, | |
| '& hr': { | |
| margin: '1.5rem 0', | |
| }, | |
| '& th': { | |
| textAlign: 'left', | |
| paddingRight: '8px', | |
| }, | |
| '& td': { | |
| paddingRight: '8px', | |
| }, | |
| '& pre': { | |
| display: 'block', | |
| padding: '9.5px', | |
| margin: '0 0 10px', | |
| fontSize: '13px', | |
| lineHeight: '1.42857143', | |
| color: '#333', | |
| wordBreak: 'break-all', | |
| wordWrap: 'break-word', | |
| backgroundColor: '#f5f5f5', | |
| border: '1px solid #ccc', | |
| borderRadius: '4px', | |
| }, | |
| '& code': { | |
| padding: '2px 4px', | |
| fontSize: '90%', | |
| color: '#c7254e', | |
| backgroundColor: '#f9f2f4', | |
| borderRadius: '4px', | |
| }, | |
| '& pre code': { | |
| padding: 0, | |
| fontSize: 'inherit', | |
| color: 'inherit', | |
| whiteSpace: 'pre-wrap', | |
| backgroundColor: 'inherit', | |
| borderRadius: 0, | |
| }, | |
| }, | |
| markdown: { | |
| '& pre': { | |
| padding: '0.375rem', | |
| color: '#333', | |
| backgroundColor: '#f5f5f5', | |
| border: '1px solid #ccc', | |
| borderRadius: '4px', | |
| }, | |
| '& code': { | |
| color: '#c7254e', | |
| backgroundColor: '#f9f2f4', | |
| }, | |
| '& pre code': { | |
| color: 'inherit', | |
| whiteSpace: 'pre-wrap', | |
| backgroundColor: 'inherit', | |
| }, | |
| }, |
|
@Forfold I realized the weird line heights were coming from the |
make checkto catch common errors. Fixed any that came up.Description:
This PR removes and refactors the css for the markdown component. Converting those styles to be "CSS in JS", we scope that css to the component which will prevent it from being left behind if and when we remove the in-app documentation.
Describe any introduced user-facing changes:
Markdown text elements will have less vertical space between each other
Note
This PR previously contained changes to the bundling configuration. In order to limit scope, those changes were reverted.