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

LG-4760: Code minimal copy button #2661

Merged
merged 100 commits into from
Jan 30, 2025

Conversation

shaneeza
Copy link
Collaborator

@shaneeza shaneeza commented Jan 24, 2025

✍️ Proposed changes

🎟 Jira ticket: LG-4760.

This PR adds a new minimal copy button.

Before:

Screenshot 2025-01-27 at 12 09 18 PM

After:

Screenshot 2025-01-27 at 12 50 45 PM

✅ Checklist

For bug fixes, new features & breaking changes

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have run pnpm changeset and documented my changes

For new components

  • I have added my new package to the global tsconfig
  • I have added my new package to the Table of Contents on the global README
  • I have verified the Live Example looks as intended on the design website.

🧪 How to test changes

@shaneeza shaneeza requested review from stephl3 and sandynguyenn and removed request for a team January 27, 2025 18:28
Base automatically changed from LG-4748-code-panel to LG-4657-code-improvements-integration January 29, 2025 17:36
@@ -0,0 +1,11 @@
const _baseQuery = (size: number) =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was copied from the select component

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it worth replacing this in Select in this body of work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

panel: undefined,
},
],
storyNames: ['Generated', 'MinimalCopyButton'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the optimal way to organize the generated stories? My understanding is that if we specify custom storyNames that 'Generated' should not be used in favor of more specific sections. However, I do see that we do that for InputOption.stories.tsx

Copy link
Collaborator

Choose a reason for hiding this comment

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

I got more context as I read further down. What do you think about calling these 'WithPanel and WithoutPanel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'll update the story names

packages/code/src/Code.stories.tsx Show resolved Hide resolved
@@ -637,6 +686,7 @@ describe('packages/Code', () => {
showLineNumbers={true}
onCopy={() => {}}
darkMode={true}
copyButtonAppearance="hover"
Copy link
Collaborator

Choose a reason for hiding this comment

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

From reading the changeset/readme, I would expect attempting to pass in copyButtonAppearance when panel is defined would result in a TS error or vice-versa. Should those types use a discriminated union?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that makes a lot of sense; I'll update that

Comment on lines 52 to 57
hasPanel,
showExpandButton,
}: {
scrollState: ScrollState;
theme: Theme;
showPanel: boolean;
hasPanel: boolean;
Copy link
Collaborator

@stephl3 stephl3 Jan 29, 2025

Choose a reason for hiding this comment

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

const showPanel = !!panel || shouldRenderTempPanelSubComponent;
showPanel seems to better align with the value getting passed in over hasPanel

what's the reason for renaming to hasPanel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember, but I don't feel strongly about hasPanel. I'll switch it back.

Comment on lines 229 to 231
{!showPanel &&
copyButtonAppearance !== CopyButtonAppearance.None &&
ClipboardJS.isSupported() && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: move to variable to declutter the JSX

<CopyButton onCopy={onCopy} contents={children} />
</div>
)}

{!!panel && panel}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be using showPanel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, showPanel is true if there is a panel or if a temp panel should render because of deprecated props. The panel should only render if the consumer passes it. I updated showPanel to showPanelOrTempPanel to hopefully clarify that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh now I see that there are 2 panels! It might be more explicit to rename the old one DeprecatedPanel and then update the associated variables to *DeprecatedPanel

Comment on lines 232 to 238
<div
className={getCopyButtonWithoutPanelStyles({
copyButtonAppearance,
})}
>
<CopyButton onCopy={onCopy} contents={children} />
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe could remove a <div> here by passing className directly to CopyButton?

@@ -52,6 +61,21 @@ export type CodeProps = Omit<

language: Language | LanguageOption['displayName'];

/**
* Determines the appearance of the copy button if the panel prop is not defined. If `panel` is defined, this prop will be ignored. The copy button allows the code block to be copied to the user's clipboard by clicking the button.
Copy link
Collaborator

Choose a reason for hiding this comment

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

After reading this note again, I'm more convinced that copyButtonAppearance and panel should be a discriminated union type unless there's a specific reason to avoid that approach

@@ -0,0 +1,11 @@
const _baseQuery = (size: number) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it worth replacing this in Select in this body of work?

@@ -242,12 +225,22 @@ export const WithDeprecatedCopyableProps: StoryType<
<Code
{...(args as CodeProps)}
highlightLines={highlightLines ? [6, [10, 15]] : undefined}
copyable
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this story still be using the deprecated copyable prop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know, probably not. I'll remove it.

@shaneeza shaneeza requested a review from stephl3 January 29, 2025 23:33
Copy link
Collaborator

@stephl3 stephl3 left a comment

Choose a reason for hiding this comment

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

some docs worth updating to match the new panel | copyButtonAppearance api but overall LGTM!

I do also still see that tooltip notch issue but that can be addressed in a separate PR if you'd like


{/* @ts-expect-error - cannot pass both panel and copyButtonAppearance */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! 🥳

@@ -22,6 +24,28 @@ or
```js
panel={<Panel/>}
```

### `copyButtonAppearance`
Adds a new prop, `copyButtonAppearance`. This prop determines the appearance of the copy button if the `panel` prop is not defined. If `panel` is defined, this prop will be ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we update this language to better communicate only 1 or the other can be defined?

Adds a new slot prop, `panel`, that accepts the new `<Panel/>` sub-component. This will render the top panel with a language switcher, custom action buttons, and copy button. If no props are passed to the panel sub-component, the panel will render with only the copy button.
### `panel`

Adds a new slot prop, `panel`, that accepts the new `<Panel/>` sub-component. This will render the top panel with a language switcher, custom action buttons, and copy button. If no props are passed to the panel sub-component, the panel will render with only the copy button. This prop takes precedence over `copyButtonAppearance`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we update this language to better communicate only 1 or the other can be defined?

Comment on lines 99 to 100
| `copyButtonAppearance` | `'none'`, `'hover'`, `'persist'` | Determines the appearance of the copy button if the panel prop is not defined. If `panel` is defined, this prop will be ignored. The copy button allows the code block to be copied to the user's clipboard by clicking the button. If `hover`, the copy button will only appear when the user hovers over the code block. On mobile devices, the copy button will always be visible. If `persist`, the copy button will always be visible. If `none`, the copy button will not be rendered. | `hover` |
| `panel` | `React.ReactNode` | Slot to pass the `<Panel/>` sub-component which will render the top panel with a language switcher, custom action buttons, and copy button. If no props are passed to the panel sub-component, the panel will render with only the copy button. This prop takes precedence over `copyButtonAppearance`. | `` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we update this language to better communicate only 1 or the other can be defined?

Comment on lines 124 to 127
panel?: never;
}
| {
copyButtonAppearance?: never;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: if you want the tsdoc description tooltip to show the associated comment, you'll need to include it on these props. It's not sophisticated enough for discriminated union types unless you have another vscode extension. If you hover the panel variable, you should see that the comment doesn't show up

<CopyButton onCopy={onCopy} contents={children} />
</div>
)}

{!!panel && panel}
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh now I see that there are 2 panels! It might be more explicit to rename the old one DeprecatedPanel and then update the associated variables to *DeprecatedPanel

@shaneeza shaneeza requested a review from stephl3 January 30, 2025 16:55
Copy link
Collaborator

@stephl3 stephl3 left a comment

Choose a reason for hiding this comment

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

had a note about the tooltip notch styles which are working great but could use some final cleanup before merging :)


Adds a new slot prop, `panel`, that accepts the new `<Panel/>` sub-component. This will render the top panel with a language switcher, custom action buttons, and copy button. If no props are passed to the panel sub-component, the panel will render with only the copy button.

**_Note: This prop takes precedence over `copyButtonAppearance`. Either use `copyButtonAppearance` or `panel`, not both._**
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could just be me, but the mentions of "... takes precedence over..." + "this prop will be ignored" seem to suggest a consumer could define both if they wanted to although now with the discriminated union they should only be able to define one or the other

Simplifying the language to something like "x cannot be used with y" + "y cannot be used with x" reads more straightforward to me. Overall, I'm good with these as they are and will leave it up to you!

Comment on lines +29 to +32
div[role='tooltip'] svg {
width: 26px;
height: 26px;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move this into tooltipStyles on L6? or if it needs to be defined here, can we remove tooltipStyles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh whoops, i forgot to remove tooltipStyles

@shaneeza shaneeza merged commit b0053aa into LG-4657-code-improvements-integration Jan 30, 2025
2 of 5 checks passed
@shaneeza shaneeza deleted the LG-4760/code-copy-button branch January 30, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants