design-system icon indicating copy to clipboard operation
design-system copied to clipboard

`CodeBlock` - height toggle feature (HDS-4745)

Open KristinLBradley opened this issue 8 months ago • 2 comments

WIP

:pushpin: Summary

If merged, this PR adds a height toggle control feature for expanding the height of the CodeBlock which appears when a maxHeight value is set that exceeds by the content height.

Showcase: https://hds-showcase-git-hds-4745-codeblock-height-toggle-hashicorp.vercel.app/components/code-block#height-limit

:hammer_and_wrench: Detailed description

Completed:

  • Main functionality
  • Integration tests
  • Showcase examples
  • Styling/behavior following current design consensus (designs not final)

TODO:

  • Implement finalized designs once ready
  • Add accessibility features
  • Update/add to related documentation

:link: External links


👀 Component checklist

  • [ ] Percy was checked for any visual regression
  • [x] A changelog entry was added via Changesets if needed (see templates here)

:speech_balloon: Please consider using conventional comments when reviewing this PR.

KristinLBradley avatar Apr 16 '25 01:04 KristinLBradley

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview May 12, 2025 3:31pm
hds-website ✅ Ready (Inspect) Visit Preview May 12, 2025 3:31pm

vercel[bot] avatar Apr 16 '25 01:04 vercel[bot]

Implemented designs are at a point where @hashicorp/hds-design feedback would be welcome.

(You can take a look at how the current behavior is looking, ect. to inform decisions for finalizing designs.)

KristinLBradley avatar Apr 18 '25 23:04 KristinLBradley

A few thing that I've noticed in the showcase of this PR, and I don't see in the "production" showcase:

Highlight rectangles not in the right place screenshot_4932

Something strange with the line numbers screenshot_4933

Small offset of the highlight rectangles screenshot_4934

didoo avatar May 01 '25 21:05 didoo

A few thing that I've noticed in the showcase of this PR, and I don't see in the "production" showcase:

@didoo Thanks for catching those. The line highlight issues have been fixed. For the line number issue inside the tabs, I do see this issue myself in production. The issue seems to be that the setPrismCode function does not get run when the code block becomes visible.

I can address this issue in a separate pr, since it's not related to anything here, to not hold up Kristin's feature. It will require heavier structural changes. I tested adding an IntersectionObserver to trigger the update and that seems to fix it.

dchyun avatar May 02 '25 14:05 dchyun

A few thing that I've noticed in the showcase of this PR, and I don't see in the "production" showcase:

@didoo Thanks for catching those. The line highlight issues have been fixed. For the line number issue inside the tabs, I do see this issue myself in production. The issue seems to be that the setPrismCode function does not get run when the code block becomes visible.

I can address this issue in a separate pr, since it's not related to anything here, to not hold up Kristin's feature. It will require heavier structural changes. I tested adding an IntersectionObserver to trigger the update and that seems to fix it.

I have a PR #2856 to fix the above issues, and branched off of here to avoid any conflicts. Don't think those fixes are needed to merge this feature, but if they look good can get them all together.

dchyun avatar May 06 '25 12:05 dchyun

Suggestion: It would be nice to make the shadow not overlap the scrollbar. Advanced Table does it for the scroll indicators.. since youre already getting dimensions of the code block I think its worth it.

Screenshot 2025-05-05 at 4 07 02 PM

@shleewhite Could you please share what browser this is in? Before, I thought I had this working (a fix to prevent shadow overlap)

KristinLBradley avatar May 06 '25 20:05 KristinLBradley

Suggestion: It would be nice to make the shadow not overlap the scrollbar. Advanced Table does it for the scroll indicators.. since youre already getting dimensions of the code block I think its worth it. Screenshot 2025-05-05 at 4 07 02 PM

@shleewhite Could you please share what browser this is in? Before, I thought I had this working (a fix to prevent shadow overlap)

@KristinLBradley it was chrome with scrollbars set to always visible as a macOS setting.

shleewhite avatar May 06 '25 20:05 shleewhite

Suggestion: It would be nice to make the shadow not overlap the scrollbar. Advanced Table does it for the scroll indicators.. since youre already getting dimensions of the code block I think its worth it. Screenshot 2025-05-05 at 4 07 02 PM

@shleewhite Could you please share what browser this is in? Before, I thought I had this working (a fix to prevent shadow overlap)

@KristinLBradley I originally had changed your structure to have the gradient attached to the button element, which caused this issue. I reverted back to your approach of using a pseudo element.

dchyun avatar May 06 '25 21:05 dchyun

Suggestion: It would be nice to make the shadow not overlap the scrollbar. Advanced Table does it for the scroll indicators.. since youre already getting dimensions of the code block I think its worth it. Screenshot 2025-05-05 at 4 07 02 PM

@shleewhite Could you please share what browser this is in? Before, I thought I had this working (a fix to prevent shadow overlap)

@KristinLBradley I originally had changed your structure to have the gradient attached to the button element, which caused this issue. I reverted back to your approach of using a pseudo element.

@dchyun Thanks, was there a reason to change it before?

Btw, I'm now seeing unnecessary horizontal scroll bars in some Showcase examples that weren't there before.

image

I think I have a fix for the horizontal scrolling. I'll comment near the relevant lines of code. See this comment: https://github.com/hashicorp/design-system/pull/2826/files#r2076472178

KristinLBradley avatar May 06 '25 21:05 KristinLBradley

@dchyun Thanks, was there a reason to change it before?

@KristinLBradley the original approach was causing issues with the line highlighting that Prism.js needed to apply.

dchyun avatar May 07 '25 12:05 dchyun

@dchyun Thanks, was there a reason to change it before?

@KristinLBradley the original approach was causing issues with the line highlighting that Prism.js needed to apply.

@dchyun Thanks, I think that was due to how I had restructured how the padding was applied?

KristinLBradley avatar May 07 '25 20:05 KristinLBradley

@dchyun Thanks for adding the fixes for the horizontal overflow issues. It's looking much better.

I am actually noticing now though that removing the right side padding does make a difference when you scroll all the way to the right of code that has horizontal overflow. I'll try experimenting a bit to see if I can come up with any possible solutions.

Update:

Try the following to add the right side padding the the code element instead:

.hds-code-block pre code {
    padding-right: 16px;
    display: inline-block;
}

Also within the CSS try scoping all styles applied to the code element by adding the pre parent tag" pre code. I noticed that if a code element is used within the CodeBlock header area that it's incorrectly inheriting the styles. (I tried scoping code styles using .hds-code-block__code code instead but that didn't seem to be working for some reason. You could try that too though in case it was just an in-browser glitch I was having.)

KristinLBradley avatar May 07 '25 20:05 KristinLBradley

So Dylan and I are starting to uncover a few issues related to the gradient overlay used with the overflowing code content...

If you scroll horizontally, the gradient no longer covers the sides of the code content. If you try to compensate by stretching the width of the gradient overlay to be wider that 100% then it introduces unnecessary horizontal scroll bars when not needed.

These issues are related to styles adding padding around the code content. Restructuring this padding can fix the positioning issues with the gradient overlay but it then throws off how the the JS calculates positioning for line highlighting.

I'm not sure of the best way forward to solve these issues.

We could possibly decide that we don't mind the unnecessary horizontal scrollbar or else decide that we don't mind that the gradient doesn't cover the full width of the code in the case the user scrolls horizontally when needed.

Or else we could come up with a different design solution such as Andrew's original design which added a bottom footer to contain the height toggle button vs. having the button overlay the code content. Or else we could find another way to increase contrast with the overlaid button and code content such as by using a dropshadow behind the button or something else instead of the current gradient.

Issue 1: Unnecessary horizontal scrollbar (if gradient is stretched to fix issue #2) image

Issue 2: Gradient doesn't cover both sides if user scrolls horizontally image

KristinLBradley avatar May 07 '25 23:05 KristinLBradley

I noticed this visual issue: If you scroll down when the height isn't toggled to auto: image

Shouldn't the line touch all the way at the bottom if the line number is active? Because when you toggle the auto height as active, it does the following (and should be consistent): image

majedelass avatar May 08 '25 12:05 majedelass

So Dylan and I are starting to uncover a few issues related to the gradient overlay used with the overflowing code content...

If you scroll horizontally, the gradient no longer covers the sides of the code content. If you try to compensate by stretching the width of the gradient overlay to be wider that 100% then it introduces unnecessary horizontal scroll bars when not needed.

These issues are related to styles adding padding around the code content. Restructuring this padding can fix the positioning issues with the gradient overlay but it then throws off how the the JS calculates positioning for line highlighting.

I'm not sure of the best way forward to solve these issues.

We could possibly decide that we don't mind the unnecessary horizontal scrollbar or else decide that we don't mind that the gradient doesn't cover the full width of the code in the case the user scrolls horizontally when needed.

Or else we could come up with a different design solution such as Andrew's original design which added a bottom footer to contain the height toggle button vs. having the button overlay the code content. Or else we could find another way to increase contrast with the overlaid button and code content such as by using a dropshadow behind the button or something else instead of the current gradient.

Issue 1: Unnecessary horizontal scrollbar (if gradient is stretched to fix issue #2) image

Issue 2: Gradient doesn't cover both sides if user scrolls horizontally image

@KristinLBradley Not to throw another option in the ring - but the way the advanced table shadows work is that they are a sibling of the grid and then the height/width are set appropriately and its positioned absolutely on top of the grid. that way if you scroll it always looks correct.

shleewhite avatar May 08 '25 14:05 shleewhite

@KristinLBradley Not to throw another option in the ring - but the way the advanced table shadows work is that they are a sibling of the grid and then the height/width are set appropriately and its positioned absolutely on top of the grid. that way if you scroll it always looks correct.

@shleewhite The issue is that the gradient needs to be a child of the overflowed container, and have position: sticky. This is so it can avoid overlapping the line numbers, but move with the content as a user scrolls horizontally.

We tried placing the gradient as a sibling, and using z-index to have the line numbers appear above, but then part of the code is exposed when scrolling horizontally.

Screenshot 2025-05-08 at 11 40 44 AM

dchyun avatar May 08 '25 15:05 dchyun

@dchyun Just noticed an issue with line-wrapping not working properly:

image

I think you can add display: block; to hds-code-block__code when line-wrapping is enabled to fix this:

image

Or perhaps only apply display: grid to hds-code-block__code when maxHeight with overflow is active or some combo along those lines. See what you think makes the most sense/works best...

KristinLBradley avatar May 08 '25 22:05 KristinLBradley

@dchyun Just noticed an issue with line-wrapping not working properly:

@KristinLBradley thanks for catching that. I added the display: block when line wrapping is enabled.

dchyun avatar May 09 '25 13:05 dchyun

Posted by Majed:

I noticed this visual issue: If you scroll down when the height isn't toggled to auto: image

Shouldn't the line touch all the way at the bottom if the line number is active? Because when you toggle the auto height as active, it does the following (and should be consistent): image

@majedelass There needs to be extra space at the bottom so that the "Show more code" button won't overlap the last few lines of code when you scroll to the bottom. The numbering does not extend to the bottom because the extra space at the bottom is just that (space), it's not extra lines of actual code content.

Oh sorry, you meant the line by the side of the numbers I see. That indeed should stretch to the bottom. let me take a look...

So this is an issue I had helped fix previously and I was worried it had reoccurred but I'm seeing the line stretch all the way to the bottom still as it should at least in Chrome. (Perhaps @dchyun also added another tweak for this if it had been reoccurring...)

I just checked Safari as well though, and I am seeing an inconsistent glitch happen in which there is a lot of extra space at the bottom AND the line doesn't stretch. It doesn't happen consistently though.

Safari screenshot: image

So far not seeing this glitch in either Firefox or Edge...

KristinLBradley avatar May 09 '25 15:05 KristinLBradley

I just checked Safari as well though, and I am seeing an inconsistent glitch happen in which there is a lot of extra space at the bottom AND the line doesn't stretch. It doesn't happen consistently though. So far not seeing this glitch in either Firefox or Edge...

This issue seems to not be replicable on anyone's instances of Safari, but to those reviewing please test just to double check no one else can replicate this.

dchyun avatar May 09 '25 18:05 dchyun