batches: commit signing settings UI#52907
Conversation
2b59016 to
3ade53c
Compare
3ade53c to
04989dd
Compare
st0nebreaker
left a comment
There was a problem hiding this comment.
An initial pass, left a few comments. This is very clean & easy to read, thanks Kelli!
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
| <Tooltip content="Delete GitHub App"> | ||
| <Button aria-label="Delete" onClick={onDelete} disabled={loading} variant="danger" size="sm"> | ||
| <Icon aria-hidden={true} svgPath={mdiDelete} /> Delete | ||
| </Button> | ||
| </Tooltip> |
There was a problem hiding this comment.
This tooltip was extraneous.
| interface Props extends TelemetryProps { | ||
| externalServicesFromFile: boolean | ||
| allowEditExternalServicesWithFile: boolean | ||
| } | ||
| interface Props extends TelemetryProps {} | ||
|
|
||
| export const GitHubAppPage: FC<Props> = ({ | ||
| telemetryService, | ||
| externalServicesFromFile, | ||
| allowEditExternalServicesWithFile, | ||
| }) => { | ||
| export const GitHubAppPage: FC<Props> = ({ telemetryService }) => { |
There was a problem hiding this comment.
These props were unused.
There was a problem hiding this comment.
Cleanup Kelli is here. 😄
Love this improvements to our codebase.
| <Button className="text-primary text-nowrap" onClick={onClick} variant="link" aria-label={label}> | ||
| <Button | ||
| className="text-primary text-nowrap font-weight-normal" | ||
| onClick={onClick} | ||
| variant="link" | ||
| size="sm" | ||
| aria-label={label} | ||
| > |
There was a problem hiding this comment.
Updated this button a bit to make it look more like the "View on GitHub" button/link for GH Apps.
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff b154242...4a71870.
|
f78bcac to
75f8556
Compare
st0nebreaker
left a comment
There was a problem hiding this comment.
Looks good!! ✨ Just a small comment on mobile responsiveness & a storybook update below
| > = ({ node, readOnly }) => { | ||
| const ExternalServiceIcon = defaultExternalServices[node.externalServiceKind].icon | ||
| return ( | ||
| <li className={classNames(styles.node, 'list-group-item')}> |
There was a problem hiding this comment.
Around ~790 screen width this item gets crowded & overflows (screen break depending on length of the names of course). Should we add a max-width to line break here or overflow-x?
There was a problem hiding this comment.
Oh good call, yeah we can just make it wrap when it needs to.
There was a problem hiding this comment.
Okay here's it at medium-ish size now when it starts needing to wrap:
And here's it at phone size, for those eager beaver site admins who wanna set up their GH App on the go:
| interface Props extends TelemetryProps { | ||
| externalServicesFromFile: boolean | ||
| allowEditExternalServicesWithFile: boolean | ||
| } | ||
| interface Props extends TelemetryProps {} | ||
|
|
||
| export const GitHubAppPage: FC<Props> = ({ | ||
| telemetryService, | ||
| externalServicesFromFile, | ||
| allowEditExternalServicesWithFile, | ||
| }) => { | ||
| export const GitHubAppPage: FC<Props> = ({ telemetryService }) => { |
There was a problem hiding this comment.
Cleanup Kelli is here. 😄
Love this improvements to our codebase.
| swhere := where.Query(sqlf.PostgresBindVar) | ||
| for i, arg := range where.Args() { | ||
| swhere = strings.Replace(swhere, fmt.Sprintf("$%d", i+1), fmt.Sprintf("%v", arg), 1) | ||
| } |
There was a problem hiding this comment.
Heheh in case you want more context: https://github.com/sourcegraph/sourcegraph/pull/52965#discussion_r1219954089.
Everyone's favorite ORM sqlf making it REAL HARD to just get the WHERE clause of a query as a string. 😂
There was a problem hiding this comment.
Thanks for the additional context.
I figured that was the case tbh but I don't think sqlf will offer an API for that because i think it's misuse can lead to sql injection.
This PR introduces the initial settings UI components to list out GitHub App connections. As you'll see there's lots of TODOs in here to hook up parts of the workflow or link to docs as those things become available. I decided to open the PR up at this state to avoid bloating it, so treat this mostly as a UI review. 🙂
Screenshots
As a user:
As an admin:
Demonstrating some of the clean up/consistency work:
Test plan
Storybook coverage.