-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add advanced mode toggle #15912
Add advanced mode toggle #15912
Conversation
7cd679a
to
059b522
Compare
059b522
to
5e4d671
Compare
5e4d671
to
a3a86ce
Compare
@@ -15,11 +16,12 @@ interface SettingsViewProps { | |||
const SettingsView: React.FC<SettingsViewProps> = ({ connection }) => { | |||
const { mutateAsync: deleteConnection } = useDeleteConnection(); | |||
|
|||
const [isAdvancedMode] = useAdvancedModeSetting(); |
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.
It doesn't look like there is a way for users to turn this mode on in OSS. I only see the Cloud settings page. Is that intentional?
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.
I wondered about that, too! I couldn't find any workspace settings page outside of cloud, though:
┌ ~/c/airbyte/airbyte-webapp (amb/add-advanced-mode-toggle|✔) 🌗 a3a86ce75c
└➣ fd workspace | egrep -v '^\./src/packages/cloud/' | egrep '\.[^/]+' # filter out cloud-specific results and directories
./src/core/domain/workspace/Workspace.ts
./src/core/domain/workspace/WorkspaceService.ts
./src/hooks/services/useWorkspace.tsx
./src/pages/SettingsPage/components/useWorkspaceEditor.tsx
./src/services/workspaces/WorkspacesService.tsx
When I looked through those files and the places that used them, I couldn't find any that seemed equivalent to the cloud workspace settings page (src/views/Settings/PreferencesForm/PreferencesForm.tsx
seemed to be the closest, but the other fields were concerned with updates and telemetry, rather than product feature configuration).
I'll fire up a local backend and do a bit more investigation, though, thanks for calling that out.
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.
Yeah. OSS doesn't use workspaces. Hmm. I remember chatting about this with @edmundito and @timroes at one point and I think we settled on this being a new settings page for OSS? (like Account, Sources, Destinations etc. are)
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.
Technically OSS deployments do use a single workspace under the hood (and I believe users can also use the API to create more workspaces, and switch between them by changing the workspace ID in the URL), so if we are also applying this change to OSS we will still want to key the local storage values based on workspace ID like you are doing in this PR.
But yeah, we will probably need a new settings page to contain this toggle.
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.
Cool, thanks for clarifying!
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.
Come to think of it, I think it would make some sense to put this within "Account Settings" for OSS, at least so long as the OSS version uses a singleton workspace. My thinking is that each advanced mode setting is logically scoped to a specific account/workspace pairing; the user account is implicit (settings are always changed for the logged-in user)*, so for the cloud version of the app it makes sense to put it under workspace settings. For OSS, the workspace is implicit, too (only one workspace per OSS instance), but the logical dependency on a user account is still there, so it feels natural enough to me for the OSS "Advanced Mode" setting to be in the account settings page--at least, it feels less arbitrary than a whole new "Advanced Mode"/"Viewing settings"/etc settings page.
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.
What you said makes sense. The one counterargument I have is that if some OSS user has created multiple workspaces using the API and they commonly switch between them by changing the workspace ID in the URL, then it may be slightly confusing that there is a toggle affecting specific workspaces that lives in Account Settings
, as to me Account Settings implies that these are configurations for my entire account, not an individual workspace.
However, I don't think that this is the typical way OSS users use the app - I think typically they use it as you describe, where they aren't even really aware that they are using a "workspace" and they will only ever have a single account and workspace.
Though, I could see us eventually wanting to make workspaces more of a first-class feature in the OSS app (e.g. providing a UI to create and switch workspaces similar to Cloud). Not sure what the timeline of that would be but I'm pretty sure I've heard that it is a requested feature.
All of that said, I don't have super strong feelings on this and I think either option would be fine, both with their slight tradeoffs. So I'm happy with deferring to what you think is best here!
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.
Had a few comments and suggestions but overall this is looking good!
Tested locally and it seems to be working as intended, except for the save changes
comment I left below
...e-webapp/src/packages/cloud/views/workspaces/WorkspaceSettingsView/WorkspaceSettingsView.tsx
Show resolved
Hide resolved
)} | ||
</Field> | ||
|
||
<div className={classNames(styles.formItem, styles.buttonGroup)}> | ||
<Button type="button" secondary disabled={!dirty} onClick={() => resetForm()}> |
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.
While testing locally, I noticed that if I toggle Advanced mode to be enabled, and I click save changes
and wait for that to complete, and then I click cancel
, it sets Advanced mode
back to disabled.
As a user, I was expecting cancel
to just get rid of any changes I had made since the last time I clicked save changes
, so this was unexpected for me. Is this the intended behavior?
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.
After playing with this a bit more, I've realized that if I enabled advanced mode, then navigate to another page, and come back to this settings page, then I see the opposite behavior -- I can disable advanced mode, click save, and then if I click cancel
it sets Advanced mode back to enabled.
So I think the part that's causing me confusion is that I expect save changes
to set the values I've selected as the "non-dirty" values for this form, and I expect the cancel
button to be disabled when I do so (but it is not). But what is actually happening is it seems the "non-dirty" values for the form are whatever was set when I initially loaded the page.
This likely isn't an issue that was introduced by any of your changes in your PR, but rather we may just be using Formik incorrectly
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.
As a user, I was expecting cancel to just get rid of any changes I had made since the last time I clicked save changes, so this was unexpected for me.
Whoa, I didn't expect that, either. A little further testing shows that this form's save/cancel buttons do the exact same thing with changes to the workspace name, so it's definitely a form-wide issue.
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.
@ambirdsall want to create an issue for that?
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.
Done: #16149
@@ -15,11 +16,12 @@ interface SettingsViewProps { | |||
const SettingsView: React.FC<SettingsViewProps> = ({ connection }) => { | |||
const { mutateAsync: deleteConnection } = useDeleteConnection(); | |||
|
|||
const [isAdvancedMode] = useAdvancedModeSetting(); |
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.
Technically OSS deployments do use a single workspace under the hood (and I believe users can also use the API to create more workspaces, and switch between them by changing the workspace ID in the URL), so if we are also applying this change to OSS we will still want to key the local storage values based on workspace ID like you are doing in this PR.
But yeah, we will probably need a new settings page to contain this toggle.
const onDelete = () => deleteConnection(connection); | ||
|
||
return ( | ||
<div className={styles.container}> | ||
<StateBlock connectionId={connection.connectionId} /> | ||
{isAdvancedMode && <StateBlock connectionId={connection.connectionId} />} |
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.
Would it make sense to add a brief note here in the case that advanced mode is disabled, along the lines of If you would like to view the state blob for your connection, enabled 'Advanced mode' in the Workspace settings
I think this would be helpful to inform users that advanced mode exists, and it would instruct users who currently expect to see connection state here on how they can bring it back.
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.
Good suggestion--I've been a little bothered by the "user is surprised to see connection state UI has disappeared and doesn't know how to get it back" scenario, too.
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.
I haven't found a decent reference for the sort of unobtrusive info callout this should be; I'm inclined to ask for a design and put it off implementing it for a follow-up PR.
(left this comment in drafts for an hour by accident, at which point Lake had already posted a similar finding on Slack; nonetheless, putting it up here for others/posterity)
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.
The issue with merging this before we've added this callout is that we may end up with users in the confusing scenario we described in this comment thread.
You could always try adding something simple-looking here for now and then we can ask design to give us something, at which point we can update the look. Not sure how we usually handle situations like this though.
@timroes do you have any thoughts here?
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.
Discussed this earlier offline: We think it's best not to put the warning into the product for now, and rather leverage a tool like Intercom in our cloud to show users a banner that are looking for this, so we don't have "migration code" sitting around for too long (again).
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.
Sure, makes sense. Should we set up that intercom messaging before we deploy this change 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.
I just reached out to Natalie about that, seems like the right move to me. Doing it smoothly will take a little coordination (it would be pretty weird to see a popup about how "Connection State" has gone missing right next to "Connection State" after all), so I'll need a better handle on the deployment timing than I have now, but otherwise seemslike it might be pretty straightforward.
Edit: the slack thread is here, looks like showing a popup on the connection settings page once per user (to avoid showing it to people who have already enabled advanced mode) is a go.
a3a86ce
to
f3f440a
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.
Had a few small remaining comments but otherwise this LGTM!
Tested this locally on OSS and on Cloud and it seems to be working as expected.
"settings.generalSettings.form.advancedMode.label": "Advanced mode", | ||
"settings.generalSettings.form.advancedMode.switchLabel": "Enable advanced mode", | ||
"settings.generalSettings.form.advancedMode.tooltip": "When Advanced Mode is enabled, certain views will display additional technical information.", |
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.
Is it possible to reuse the strings from the above airbyte-webapp/src/locales/en.json file for Cloud instead of duplicating them here? Maybe not depending on how our packages are set up?
const onDelete = () => deleteConnection(connection); | ||
|
||
return ( | ||
<div className={styles.container}> | ||
<StateBlock connectionId={connection.connectionId} /> | ||
{isAdvancedMode && <StateBlock connectionId={connection.connectionId} />} |
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.
Sure, makes sense. Should we set up that intercom messaging before we deploy this change then?
airbyte-webapp/src/pages/SettingsPage/pages/AccountPage/components/AccountForm.tsx
Outdated
Show resolved
Hide resolved
As of this commit, it's a bit ugly since the toggle switch has no margin between it and the text input directly above; going to migrate the page to use scss modules instead of styling it by the current page conventions.
This setting is only relevant to single-workspace views; this simplifies client code.
The switch needed a top-margin, too, so I created an scss module to hold that style. Also migrated the component-local `Header` and `Buttons` styled-components.
Keeping the className-passing behavior in LabeledSwitch, though, it seems to me to be a strictly better API for that component.
- clean up object destructuring/composition - expand inter-item spacing from 10px to 21px to match related form
f3f440a
to
b3d9c63
Compare
* Add useAdvancedModeSetting hook * Add advanced mode toggle to workspace settings As of this commit, it's a bit ugly since the toggle switch has no margin between it and the text input directly above; going to migrate the page to use scss modules instead of styling it by the current page conventions. * Query the current workspace inside useAdvancedMode This setting is only relevant to single-workspace views; this simplifies client code. * Style workspace settings form with scss module The switch needed a top-margin, too, so I created an scss module to hold that style. Also migrated the component-local `Header` and `Buttons` styled-components. * Only show connection state if advanced mode is enabled * Don't call setAdvancedMode until form is submitted * Add external label, extract text to i18n keys Keeping the className-passing behavior in LabeledSwitch, though, it seems to me to be a strictly better API for that component. * Add tooltip to advanced mode switch * Add unit test for useAdvancedModeSetting hook * Add unit test for the SettingsView component * Address PR comments - clean up object destructuring/composition - expand inter-item spacing from 10px to 21px to match related form * Add advanced mode toggle to OSS account settings * Remove commented-out button container component * mock useTrackPage since it was added to SettingsView Co-authored-by: lmossman <lake@airbyte.io>
* Add useAdvancedModeSetting hook * Add advanced mode toggle to workspace settings As of this commit, it's a bit ugly since the toggle switch has no margin between it and the text input directly above; going to migrate the page to use scss modules instead of styling it by the current page conventions. * Query the current workspace inside useAdvancedMode This setting is only relevant to single-workspace views; this simplifies client code. * Style workspace settings form with scss module The switch needed a top-margin, too, so I created an scss module to hold that style. Also migrated the component-local `Header` and `Buttons` styled-components. * Only show connection state if advanced mode is enabled * Don't call setAdvancedMode until form is submitted * Add external label, extract text to i18n keys Keeping the className-passing behavior in LabeledSwitch, though, it seems to me to be a strictly better API for that component. * Add tooltip to advanced mode switch * Add unit test for useAdvancedModeSetting hook * Add unit test for the SettingsView component * Address PR comments - clean up object destructuring/composition - expand inter-item spacing from 10px to 21px to match related form * Add advanced mode toggle to OSS account settings * Remove commented-out button container component * mock useTrackPage since it was added to SettingsView Co-authored-by: lmossman <lake@airbyte.io>
What
Adds a semi-persistent "Advanced Mode" toggle to workspace settings, with the default value of
false
; the connection state will only be displayed if "Advanced Mode" istrue
. Resolves #15145How
Adds a
LabeledSwitch
to the "General Settings" section of "Workspace Settings"; when the general settings form is saved, the selection is saved to a json object inlocalStorage
which tracks advanced mode selections for each workspace (keyed by theirworkspaceId
).To style that
LabeledSwitch
, I added a scss module to theWorkspaceSettingsView
component; having done so, I migrated all component-local styled-components to scss instead. I also edited theLabeledSwitch
component to mergeprops.className
with its root element'sclassName
; while my implementation evolved in ways that made that change unnecessary, it's a purely additive change that won't affect existing usages and makes the component a bit more flexible, so I didn't undo the change.Persisting the user's advanced mode settings to a database table instead of
localStorage
would be a much more robust long-term solution,which is left as an exercise to the readerbut this PR only adds an initial prototype solution.Recommended reading order
.../hooks/services/useAdvancedModeSetting.ts
and test file.../cloud/views/workspaces/WorkspaceSettingsView/WorkspaceSettingsView.tsx
.../pages/ConnectionPage/pages/ConnectionItemPage/components/SettingsView.tsx
and test file.../pages/AccountPage/conponents/AccountForm.tsx
.../components/LabeledSwitch.tsx
.../cloud/views/workspaces/WorkspaceSettingsView/WorkspaceSettingsView.module.scss
.../utils/testutils.tsx
🚨 User Impact 🚨
Hiding the connection status by default could potentially break certain users' workflows with no indication about how to reenable the connection status display. Otherwise, nothing is changed but the amount of information displayed, so all existing connections will be unaffected.
Tests
I'd have liked to use a Cypress test to get a very high-level assertion that the feature works as expected (and one which would require minimal change to work with a database-backed rewrite), but I decided that saving a few rabbit holes for later PRs was the better part of valor. As things stand, I have some unit tests for the
useAdvancedModeSetting
hook and for the conditional rendering of theStateBlock
component inpages/ConnectionPage/pages/ConnectionItemPage/components/SettingsView.tsx
.Unit
Screenshots
Cloud workspace general settings:
Cloud account settings (no toggle, just proving I didn't get sloppy with shared components):
Cloud connection settings, basic mode:
Cloud connection settings, advanced mode:
OSS account settings:
OSS connection settings, basic mode:
OSS connection settings, advanced mode: