batches: add entry point to GH App commit signing from repo GH Apps page#52430
Conversation
| headingElement="h2" | ||
| path={[{ text: 'GitHub Apps' }]} | ||
| className={classNames(styles.pageHeader, 'mb-3')} | ||
| description={ |
There was a problem hiding this comment.
I took the liberty of updating the layout here slightly to bring this page closer in line with the other site admin pages immediately adjacent to it. We're still awfully inconsistent in this regard across site admin pages, but at least now all the pages in this sub-section of site admin will look about the same. 😅
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 80514d0 and 60d130d or learn more. Open explanation
|
st0nebreaker
left a comment
There was a problem hiding this comment.
Looks good, thank you! Just a few minor comments below
| @@ -0,0 +1,5 @@ | |||
| .page-header { | |||
| display: grid; | |||
| grid-template-columns: 1fr auto; | |||
There was a problem hiding this comment.
Cool - I don't this style used a lot in the wild 🌲 !
| <> | ||
| {' '} | ||
| To create a GitHub App to sign Batch Changes commits, visit{' '} | ||
| <Link to="/site-admin/batch-changes">Batch Changes settings</Link>. | ||
| </> |
There was a problem hiding this comment.
Nit: This might be a bit hidden from the user because it's enmeshed with the former text; maybe it should be a new line to make the text more clear? But maybe we want it more hidden anyway? 😅
There was a problem hiding this comment.
Haha no that's a good question! I actually did put a break between them at first, but then just visually it looks kinda... off-balanced? And it suddenly becomes unclear where the "Create" button should go, because it only relates to the former line of text, which would make me want to align it to the top, but it also results in another entry on the list below, which would make me want to align it to the bottom... the disagreement over which makes me kinda just want to stick it in the middle?? Basically the description area just isn't set up well for two distinct text sections when there's also a CTA button up there. 😞 So I'm kinda tempted to just leave it all together, even at the risk of it being too subtle. 👀
There was a problem hiding this comment.
but it also results in another entry on the list below, which would make me want to align it to the bottom
Do you mean the 2nd line of text? I think top alignment would look okay, if I'm following right. But I don't have too strong an opinion here; keeping it more compact looks good to me too.
| return ( | ||
| <Routes> | ||
| <Route index={true} element={<GitHubAppsPage />} /> | ||
| <Route index={true} element={<GitHubAppsPage batchChangesEnabled={props.batchChangesEnabled} />} /> |
There was a problem hiding this comment.
Do we have the batchChangesEnabled flag off automatically for licensed users? I'm wondering if this field would be better determined from the license in case admin users forget about the batch changes feature. What do you think? In #52113 I've been exploring our batches licensing a lot so this flags it 😆
There was a problem hiding this comment.
Hehe I'm thinking I want to come back to this when we do some clean up and pick up https://github.com/sourcegraph/sourcegraph/issues/52207. 😉 For now I just wanted to match the behavior to how we determine to show the "Batch Changes" section of site admin settings, and for now that's purely derived based on this antiquated-- ahem... I mean, legacy property... 😅
Closes https://github.com/sourcegraph/sourcegraph/issues/52342.
Based on the discussion and resolution over Slack, we don't want the Batch Changes GitHub App to live on the main GH Apps page since their purposes and connection flow differ in most regards. However, it might still be natural for admins to expect to find a Batch Changes GH App on this page, or to wind up here by mistake when trying to set up a GH App for commit signing.
As such, this PR adds a note and link to the Batch Changes settings page in the page header description, to serve as a secondary entry point into the future GH App commit signing flow.
When the Batch Changes feature is disabled on an instance, this additional text will not be present.
Obviously, there's nothing on the Batch Changes settings page for this yet; this PR is just for adding the link.
Test plan
Manual testing:
batchChangesEnabled: false, verified the description appeared as-is.batchChangesEnabled: true, verified the updated description appeared and the link routed me to the BC site admin settings page.