Fix copy code button size when over oneline code blocks#2007
Fix copy code button size when over oneline code blocks#2007
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
trichoplax
left a comment
trichoplax
left a comment
There was a problem hiding this comment.
I'm away from my development machine today so I haven't tested either the code or my suggestion, so my comments here are just thoughts.
| const $content = $(this).text(); | ||
| const numLines = $content.trim().split(/\r?\n/).length | ||
|
|
||
| if (numLines <= 1) { |
There was a problem hiding this comment.
Using <= 1 suggests we are considering not only the case where numLines is 1, but also the case where numLines is 0. Should that case avoid having the Copy button (of either size)?
This seems a very rare case. The only example I can think of where avoiding having the Copy button might be a problem is Code Golf. In the esolang (esoteric programming language) Whitespace it's possible to have a valid program composed entirely of newlines, tabs, and spaces, which would all be removed by the .trim() leaving zero length, but still needing a Copy button.
If we wanted to cover both of these rare cases we could also test the length of the content:
- For zero length content, show no button.
- For zero lines after trimming, but non-zero length content, show the small or large Copy button depending on number of newlines.
Is there a simpler approach where we find the height of the code block without trimming? Since both \r\n and \n contain one \n, could we just count how many times \n occurs in the content to determine the number of lines, and choose a button accordingly:
- 0 lines: No button
- 1 line: Small button
- Anything else: Large button
Maybe something like (untested):
numLines = $content.length == 0 ? 0 : $content.match(/\n/g).length + 1| $(".post--content pre > code") | ||
| const buttonTemplate = `<button class="copy-button button is-muted is-outlined has-margin-2">Copy</button>`; | ||
|
|
||
| $('.post--content pre > code') |
There was a problem hiding this comment.
Non-blocking: would this be a good opportunity to phase out jQuery for this file?
cellio
left a comment
cellio
left a comment
There was a problem hiding this comment.
Tested and looks fine to me. Deferring to others for code review. During testing I wondered if the smaller button would actually be fine everywhere, but I didn't test on a phone and maybe the smaller button is a more difficult target there?
3 participants
With this PR, multiline blocks will still show the big button as before:
Single-line blocks, however, will show a smaller better-fitting version of the button:
closes #2002