Skip to content
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

CodeBlock: update to support dynamic content #1853

Merged
merged 13 commits into from Jan 16, 2024

Conversation

KristinLBradley
Copy link
Contributor

@KristinLBradley KristinLBradley commented Dec 4, 2023

📌 Summary

If merged, this PR:

  • updates the CodeBlock component to enable it to render updated dynamic content
  • addresses an issue where line numbers are misaligned due to an issue with font declaration
  • suppresses a build warning caused by parsing Ember comments in the component template.

Limitations:

  • dynamic content does not work with @hasLineNumbers={{true}}; this comes from a limitation with the line-numbers Prism plugin where it can only be instantiated/called once.
  • @hasLineNumbers={{true}} does not work as expected when used in a Tabs tab that is not visible at render time.

Showcase: https://hds-showcase-git-test-codeblock-with-dynamic-content-hashicorp.vercel.app/components/code-block

📸 Screenshots

Misalignment issue in HCP. The more line there are the more you can see the issue.

BeforeAfter
01-hcp-line-before 02-hcp-line-after

🔗 External links


👀 Component checklist

  • Percy was checked for any visual regression
  • A11y tests have been run locally (yarn test:a11y --filter="COMPONENT-NAME")
  • If documenting a new component, an acceptance test that includes the a11yAudit has been added
  • A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Dec 4, 2023

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Jan 16, 2024 0:08am
hds-website ✅ Ready (Inspect) Visit Preview Jan 16, 2024 0:08am

code,
element,
});
}, 100);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using next to replace this setTimeout and it still functions.

I tried using schedule instead and got errors but maybe I was using it wrong so I'll experiment a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed up the variation using next

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then let's wait for @alex-ju to come back from PTO and we can then decide how to take it from here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using next sounds like a great idea to me (not sure how I've not thought about it) – do all the examples work as expected with this change, or is it something that prevents us from moving forward with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work although I can do more testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we could also test this change to make sure it addresses the actual need in boundary (where you identified the issue) that would be even better – let me know if I can be of help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-ju I tried figuring out how to add an integration test but haven't been able to figure out a way to do it.

I can try to test it just in Boundary next though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! I can look into the integration test bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the CodeBlock updates in Boundary and everything worked as expected. The content updates dynamically.

.dom('#test-code-block pre code')
.hasText("console.log('Hello world');");
this.set('value', "console.log('Lorem ipsum');");
await settled();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially tried with rerender, but it's too 'quick' (in a sense that it doesn't wait for the whole promise to end) so I ended up using settled.

@alex-ju alex-ju force-pushed the test-codeblock-with-dynamic-content branch from 9be22d4 to b7a1e14 Compare December 18, 2023 17:41
This was caused by incorrect `font-family` declaration
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments.

@value={{this.codeValue}}
@language="javascript"
@hasCopyButton={{true}}
@hasLineNumbers={{false}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that if you set this to "true" (and probably we should) when the lines gets updated, the line numbers don't.

Copy link
Member

@alex-ju alex-ju Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, no – I'll look and see if I can find a workaround for this – doesn't look great

Copy link
Member

@alex-ju alex-ju Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this during the first week of January and today again. Unfortunately, I can not see a way in which the @hasLineNumbers feature would work with dynamic content.

For this reason, I suggest the pragmatic approach of fixing the immediate issues (I updated the PR description to highlight what this PR covers and what doesn't) to address current instances in HCP and Boundary then leave the hasLineNumbers issue for future evaluation (it may require us to fork the addon or move away from Prism altogether). If you find this approach acceptable, I can create a ticket to track this use case and update the CodeBlock guidance to call out the current limitations.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@hashibot-hds hashibot-hds added the docs-website Content updates to the documentation website label Jan 16, 2024
@alex-ju alex-ju merged commit 6de9afa into main Jan 16, 2024
15 checks passed
@alex-ju alex-ju deleted the test-codeblock-with-dynamic-content branch January 16, 2024 16:21
@hashibot-hds hashibot-hds mentioned this pull request Jan 16, 2024
@alex-ju alex-ju mentioned this pull request Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants