Feat: Setting as pages #3813
Feat: Setting as pages #3813
Conversation
| @@ -21,6 +21,71 @@ export const defaultZapierWebhookUrl = | |||
| 'https://hooks.zapier.com/hooks/catch/12120532/2m4okri/' | |||
| export const guideBillingUrl = 'https://speckle.guide/workspaces/billing.html' | |||
|
|
|||
| export const settingsRoutes = { | |||
| user: { | |||
| profile: { | |||
There was a problem hiding this comment.
Both route and name are needed, because they are used in different scenarios (for example checking if a route by name needs SSO access)
| @@ -123,3 +123,11 @@ export const workspaceWizardRegionQuery = graphql(` | |||
| } | |||
| } | |||
| `) | |||
|
|
|||
| export const workspaceGetIdBySlugQuery = graphql(` | |||
There was a problem hiding this comment.
Some queries are not possible with workspaceBySlug, as nested properties in some cases need an ID and don't work with slug, so this call is done first to get the id
There was a problem hiding this comment.
There used to be that hack where we just take the workspace's id from the apollo cache, cause it should already be there from the access checks done in middlewares. Can we do that here too?
There was a problem hiding this comment.
Do you remember which hack this was, or maybe @andrewwallacespeckle knows? I vaguely remember it, but dont remember where we are doing this
| title: 'General', | ||
| component: SettingsWorkspaceGeneral, | ||
| getRoute: (slug?: string) => | ||
| slug ? settingsRoutes.workspace.general.route(slug) : '', |
There was a problem hiding this comment.
Can you explain this? Seems like it needs a slug to be able to build the route, so why not make it non-optional? If its because you're typing it as SettingsMenuItem[] where all entries have the same type, then maybe we shouldn't do that?
If you just want to assert that it should fit that type, but may also have extra typing info besides that you can do [..] satisfies SettingsMenuItem[]
packages/frontend-2/components/settings/workspaces/security/sso/Wrapper.vue
Outdated
Show resolved
Hide resolved
| @@ -27,6 +27,15 @@ import { | |||
| } from '~~/lib/server-management/graphql/queries' | |||
| import { useQuery } from '@vue/apollo-composable' | |||
|
|
|||
| definePageMeta({ | |||
| middleware: ['auth', 'settings', 'admin'], | |||
There was a problem hiding this comment.
You shouldn't need to define these on every single settings page - not only that's WET, but someone will definitely at some point forget to add them to a new one. If the settings pages are correctly set up as nested pages from a top-level settings.vue page (that embeds further pages through <NuxtPage>), you can just apply these middlewares once to that
There was a problem hiding this comment.
There was a problem hiding this comment.
Same goes for 'admin' - assuming all admin pages go under settings/server/*, you can just add that middleware to server.vue
There was a problem hiding this comment.
Afaik this will also create a route for /settings, /user & /server, do you know if there a way to avoid it? I can redirect those, but I don't know if that's worth the trade-off of just defining the middleware
| @@ -8,20 +8,20 @@ | |||
| class="mb-6" | |||
| /> | |||
| <LayoutTabsHorizontal v-model:active-item="activeTab" :items="tabItems"> | |||
| <template #default="{ activeItem }"> | |||
| <template v-if="workspace" #default="{ activeItem }"> | |||
There was a problem hiding this comment.
I dislike the fact that you need to load to completion the workspace GQL query before you can even start mounting the subsequent components and their queries. This is why I think I'd rather rely on the apollo.cache hack
| @@ -116,9 +110,9 @@ | |||
| </div> | |||
|
|
|||
| <SettingsWorkspacesSecurityDomainRemoveDialog | |||
| v-if="removeDialogDomain" | |||
| v-if="removeDialogDomain && workspace" | |||
There was a problem hiding this comment.
ditto, every time we do v-if="workspace" on a component we're making the components load in serially, so if they have their own queries they're executed serially instead of in parallel
| const mutationResult = await apollo.mutate({ | ||
| mutation: SettingsUpdateWorkspaceSecurityDocument, | ||
| variables: { | ||
| input: { | ||
| id: props.workspaceId, | ||
| id: workspace.value?.id, | ||
| discoverabilityEnabled: newVal | ||
| } | ||
| }, | ||
| optimisticResponse: { |
There was a problem hiding this comment.
can we at least add a FIXME here to clean this up? the optimisticResponse still doesn't make any sense to me and cache.modify should be replaced with modifyObjectField
There was a problem hiding this comment.
also: if workspace.value?.id is gonna be undefined (i see the optional chaining), this will probably fail
This PR moves away from the settings modal in favor of having them as pages. Some clean-up and bugfixes have been done for the content overall no changes have been made. There is still some room for improvement there, but we can address these in follow-up PRs