feat: create Code Block Package #2641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: c4d5e48 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
Status: Do Not Merge
github-actions
bot
added
Area: Components
✅ Deploy Preview for paste-theme-designer ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c4d5e48:
|
|
Size Change: +745 B (0%) Total Size: 759 kB
ℹ️ View Unchanged
|
✅ Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
github-actions
bot
added
Area: Doc Site
shleewhite
commented
Aug 27, 2022
packages/paste-core/components/code-block/__tests__/index.spec.tsx
Outdated
Show resolved
Hide resolved
shleewhite
commented
Aug 27, 2022
shleewhite
commented
Aug 27, 2022
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
shleewhite
commented
Aug 27, 2022
shleewhite
changed the title
sandsng
reviewed
Aug 29, 2022
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
shleewhite
force-pushed
the
component/code-block
branch
from
4928d7e
to
aec5a9a
Compare
TheSisb
reviewed
Aug 30, 2022
packages/paste-libraries/syntax-highlighter/stories/index.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/components/code-block/__tests__/index.spec.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+37
to
+39
| MozTabSize: '4', | ||
| OTabSize: '4', | ||
| tabSize: '4', |
There was a problem hiding this comment.
question: what does this translate to?
There was a problem hiding this comment.
It changes the size of the tab character: https://developer.mozilla.org/en-US/docs/Web/CSS/tab-size
shleewhite
force-pushed
the
component/code-block
branch
from
11c6704
to
29ebf59
Compare
kelseycavitt
previously approved these changes
Sep 1, 2022
ghost
previously approved these changes
Sep 1, 2022
| "CodeBlockTab": "@twilio-paste/core/code-block", | ||
| "CodeBlockTabList": "@twilio-paste/core/code-block", | ||
| "CodeBlockTabPanel": "@twilio-paste/core/code-block", | ||
| "CodeBlockWrapper": "@twilio-paste/core/code-block", |
There was a problem hiding this comment.
for my own edification what do these changes in the .cache/mappings.json file do?
There was a problem hiding this comment.
This is for a codemod that helps people transition from barreled to unbarreled imports. It is used here: https://github.com/twilio-labs/paste/blob/main/packages/paste-codemods/transforms/barreled-to-unbarreled.js to check what people are importing and change it from import {whatevs} from '@twilio-paste/whatevs' to import {whatevs} from '@twilio-paste/core/whatevs'
| @@ -0,0 +1,9 @@ | |||
| declare module 'react-syntax-highlighter/dist/esm/languages/prism/shell-session' { | |||
| const language: any; | |||
There was a problem hiding this comment.
@TheSisb @gloriliale do either of you have context for why this file is needed?
There was a problem hiding this comment.
@shleewhite typically I write an ambient module declaration like this when a 3rd party package doesn't ship with its own type annotations or a type repository like DefinitelyTyped doesn't have community types available. That all said my guess is that this package react-syntax-highlighter was missing some types so we added them. Going a bit further to my original question, generally we'd want to avoid any types if possible, even preferring something like unknown if we don't have time to determine which type it should be.
| if (variant !== 'primary' && variant !== 'secondary' && variant !== 'reset') { | ||
| throw new Error(`[Paste: Button] <Button as="a"> only works with the following variants: primary or secondary.`); | ||
| if (variant !== 'primary' && variant !== 'secondary' && variant !== 'reset' && variant !== 'inverse') { | ||
| throw new Error(`[Paste: Button] <Button as="a"> only works with the following variants: primary and secondary.`); |
There was a problem hiding this comment.
its interesting that we include usage enforcement code like this as part of the run time! One thing we might explore in the future is handling this kind of thing through dev time only type annotations or react propTypes! cc @TheSisb
There was a problem hiding this comment.
Totally.. I don't have full context here but I think we usually try to do this stuff with types/propTypes, but for really important things we actually throw errors. We throw errors in Avatar and Badge too.
| export const CopyButton: React.FC<CopyButtonProps> = ({ | ||
| text, | ||
| i18nCopyLabelBefore = 'Copy code block', | ||
| i18nCopyLabelAfter = 'Copied!', |
There was a problem hiding this comment.
oh are these english strings default values? we don't worry about I18n english leaks like this to allow for a more minimal default API perhaps?
There was a problem hiding this comment.
Can you clarify what you mean by i18n english leaks?
There was a problem hiding this comment.
oh just that for products that are internationalized, when a developer does not provide a translated string here the UI will show in english which could be incorrect. Some design systems will enforce that all strings are injected and provide no defaults to avoid that.
gloriliale
reviewed
Sep 2, 2022
|
|
||
| type CodeBlockVariants = 'multi-line' | 'single-line'; | ||
|
|
||
| export interface CodeBlockProps extends Partial<Omit<HTMLDivElement, 'children'>> { |
There was a problem hiding this comment.
suggestion(non-blocking): I'd like to figure out if this is the best possible way to get to the type we're looking for. Way out of scope for here, but just wanted to note that somewhere.
There was a problem hiding this comment.
Totally, I kinda just guessed on how to do this.
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
nkrantz
reviewed
Sep 2, 2022
packages/paste-core/components/code-block/src/ExternalLinkButton.tsx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/components/code-block/index.mdx
Outdated
Show resolved
Hide resolved
| {...safelySpreadBoxProps(props)} | ||
| ref={ref} | ||
| element={element} | ||
| as={as} |
There was a problem hiding this comment.
question: passing h1/h2/h3/etc doesn't change the CodeBlockHeader visually, so why do we allow for the different options? The docs say "use the correct heading level" to ensure a11y but in what situation would one be correct over another? Do we really want people to have h1 code block headers?
There was a problem hiding this comment.
It all depends on the heading hierarchy of the page. I doubt there would ever be an H1 code block header, but I can totally believe other heading levels - it all depends on the other headers on the page. For example, if the code block is underneath an H3, then the code block heading should be an h4.
You can read more about heading levels here: https://www.w3.org/WAI/tutorials/page-structure/headings/
There was a problem hiding this comment.
totally non-blocking but might be helpful to add a bit of this context to the a11y section of the docs page
nkrantz
previously approved these changes
Sep 2, 2022
Co-authored-by: Shadi <TheSisb@users.noreply.github.com> Co-authored-by: gloriliale <77117846+gloriliale@users.noreply.github.com>
shleewhite
force-pushed
the
component/code-block
branch
from
78a9daa
to
f14c71b
Compare
shleewhite
dismissed stale reviews from nkrantz, ghost
, and kelseycavitt
via
214bc65
TheSisb
approved these changes
Sep 6, 2022
nkrantz
approved these changes
Sep 6, 2022
kelseycavitt
approved these changes
Sep 6, 2022
shleewhite
added
the
🕵🏻♀️ Run website visual regression
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes:
@twilio-paste/code-blockpackage, which includes:@twilio-paste/syntax-highlighterlibrary: all it does is re-exportreact-syntax-highlighterwith the languages we supportContributing to Twilio