-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Enable SSH Generator Feature Flag and polish UI #18814
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
base: main
Are you sure you want to change the base?
Conversation
f650f45
to
b963d60
Compare
172893b
to
8f66ed3
Compare
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.
Does this add the SSH profiles by default already? I know we've in the past talked about adding profiles only when the user wants them, so I was wondering if we're making that part of 1.24...
<stage>AlwaysDisabled</stage> | ||
<alwaysEnabledBrandingTokens> | ||
<brandingToken>Dev</brandingToken> | ||
<brandingToken>Canary</brandingToken> | ||
<brandingToken>Preview</brandingToken> | ||
</alwaysEnabledBrandingTokens> |
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.
Seems like this should be AlwaysEnabled
then?
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.
Nope! The SSH code is hidden behind a feature flag on Stable
builds so I'm making sure we still don't expose it yet. I want to make sure the experience feels right with the Extensions page. Once the Extensions page goes to Stable
, we can just remove the feature flag altogether.
WindowsInbox
is also one that's omitted. Same deal there.
Yeah, the profiles are added by default, but can be managed using The concept of "disabling by default" is a bit difficult here because we'd have to patch Happy to add that change in, but I just want to make sure we have consensus. I'll start a thread. |
b963d60
to
f25e6fe
Compare
8f66ed3
to
92bccdf
Compare
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.
Let's hold for the discussion.
Some other ideas from segoe fluent icons: I'm guessing we want to have the generator icon and profile icons be the same (which is an easy fix now). |
Let's do PC1 for the profiles |
oh, i'm OK with the profile and the generator having different icons! |
Enables the SSH Dynamic Profile Generator feature flag now that the Settings UI is better equipped to handle it. Since the generator was first introduced, the settings UI has grown to include an Extensions page to disable the generator (if desired) as well as a New Tab Menu page to manage how to display the generated profiles in the dropdown.
Updated the profile generator and profile icons to use font icons (StorageNetworkWireless and PC1 respectively).
This also required a minor change to the extensions page template selector. Using our icon converter sets a font size on font icons, which results in a small icon. Removing that font size setter is risky (I tried it and it resulted in clipping font icons whenever they're used in the profile dropdowns). So instead, I just created a separate entry in the template selector that creates proper styling for the font icon if it's detected.
Closes #15007