Skip to content

fix: template permissions page never loads #5014

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

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

Kira-Pilot
Copy link
Member

(hopefully) resolves #4953

Previously, on the Template Permissions page, we always showed a loader if a user did not have permissions to update the template.
We don't anymore; we just disable the dropdowns on the page:

Screen Shot 2022-11-10 at 4 13 31 PM

In addition, I made an effort to clean up loading on this page overall. @BrunoQuaresma could you please review carefully? I think you were the last person to touch this code.

@Kira-Pilot Kira-Pilot requested a review from a team as a code owner November 10, 2022 21:15
@Kira-Pilot Kira-Pilot requested review from code-asher and BrunoQuaresma and removed request for a team and code-asher November 10, 2022 21:15
@@ -152,65 +140,59 @@ export const TemplateLayout: FC<PropsWithChildren> = ({ children }) => {
}
>
<Stack direction="row" spacing={3} className={styles.pageTitle}>
{!isLoading && (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we were wrapping all these blocks in isLoading - seems like we just need to make sure the template is there (and we do on line 114).

@@ -26,14 +25,11 @@ export const TemplatePermissionsPage: FC<
context: { templateId: template?.id },
})
const { templateACL, userToBeUpdated, groupToBeUpdated } = state.context
if (!template || !permissions) {
return <Loader />
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need another loader here, right? We already have the Suspense in the parent component with a fallback loader. Also, at this point, we can be certain template is loaded.

@BrunoQuaresma
Copy link
Collaborator

Thanks for those changes! I QA it and it is working as expected.

@Kira-Pilot Kira-Pilot merged commit 3fb7892 into main Nov 14, 2022
@Kira-Pilot Kira-Pilot deleted the template-perm-loading-fix/kira-pilot branch November 14, 2022 17:33
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

navigating to /templates/<name>/permissions keeps spinning
2 participants