-
Notifications
You must be signed in to change notification settings - Fork 875
feat(site): add numbers to SSHButton content for clarity #15661
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
feat(site): add numbers to SSHButton content for clarity #15661
Conversation
if we're to stylize this as a list, we should probably do it semantically ( |
const SSHStep = (props: { helpText: string; codeExample: string }) => ( | ||
<li style={{ listStylePosition: "inside" }}> | ||
<HelpTooltipText style={{ display: "inline" }}> | ||
<strong css={styles.codeExampleLabel}>Connect to the agent:</strong> | ||
</HelpTooltipText> | ||
<CodeExample secret={false} code={props.codeExample} /> | ||
</li> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing @aslilac 🙏
Changed to use semantic HTML <li>
tags (did it naively to reduce changes, but agree semantic html is better)
recheck (retriggered CLA job, since it failed) Edit: nope, that didn't work; looking into what's going on. @benwinding did you sign the CLA? |
Hi @dannykopping 👋
Sorry just getting permission from my company to sign the CLA, will comment an update when it's signed 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we declare the props type as a separate statement, use the FC
type, and use argument destructuring to get the props values, like so:
export interface SSHButtonProps {
workspaceName: string;
agentName: string;
sshPrefix?: string;
}
export const SSHButton: FC<SSHButtonProps> = ({
workspaceName,
agentName,
sshPrefix,
}) => {
SSHStep
should do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matifali I already reviewed this, I'm waiting for the last review to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we declare the props type as a separate statement, use the
FC
type, and use argument destructuring to get the props values, like so:
Still needs this addressed, but otherwise looks good!
@dannykopping @deansheather this pr is ready to go, we're just waiting on the cla. can one of you help with that since the bot isn't working? |
Hi @benwinding could you please comment the following once you've read and agree to the CLA document?
We've have some issues with the CLA bot over the last week or so, so I apologize that we have to do it the manual way |
I have read the CLA Document and I hereby sign the CLA |
All contributors have signed the CLA ✍️ ✅ |
recheck |
Thanks for your contribution! Sorry about the difficulties merging due to the CLA bot issues 🙇 |
Summary
So when I first used this, I stupidly didn't realize that you needed to do both steps. This PR adds numbers to the commands to make that a little clearer 🙏
Screenshots