-
Notifications
You must be signed in to change notification settings - Fork 0
UI Improvements for GC Landing page #35
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
Conversation
IamJeffG
left a comment
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.
@rudokemper I think you will do a better review of the Vue code than I am able. Can I punt to you?
i18n/locales/en.json
Outdated
| "userManagement": "User Management", | ||
| "secureAccessRequired": "Secure Access Required", | ||
| "pleaseSignInToAccess": "Please sign in to access your community tools" | ||
| "pleaseSignInToAccess": "Please sign in to access your partner tools" |
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.
This change seems awkward to me. Did someone ask for it?
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 I remember a conversation with Rudo about how not everyone we support is a community, some are "partners"
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 don't think "partner" is quite right either. If someone self-deploys GC, they are not our partner.
Maybe just "access your tools"
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 this file needed? if so, can you at least rename it?
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.
deleted
middleware/oauth.global.ts
Outdated
|
|
||
| // if (loggedIn.value && to.path === "/login") { | ||
| // return router.push("/"); | ||
| // } |
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 this intended to be commented 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.
I've restored
|
@IamJeffG @conservationtimothy Yes, I will review this. |
rudokemper
left a comment
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.
Largely looks good but a handful requested changes throughout.
| <p class="text-gray-400"> | ||
| <p class="text-gray-600"> | ||
| {{ t('services.noServicesDescription') }} | ||
| </p> |
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.
Below this (glancing at the code, not sure if it's exactly here or a higher level of <div> containment), can we add a helpful paragraph (localized text), maybe italicized:
Need help? Visit the Guardian Connector documentation website. (link to docs.guardianconnector.net)
| @@ -0,0 +1,126 @@ | |||
| <script lang="ts" setup> | |||
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 use the same icon on UserManagement
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.
Also, Auth0Login has a warning complaining about the absence of LanguagePicker component. Could implement this there too to resolve that warning.
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.
In fact, I now see there is also a LanguagePicker. Can we just use one?
I'm not sure and haven't looked into it, but I suspect this warning for the Auth0Login page could also be related:
WARN [Vue warn]: Component <Anonymous> is missing template or render function.
at <Anonymous theme="white" >
at <Auth0Login error-message="" >
at <Login onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< undefined > >
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 this is kinda nicer I think since we've redesigned maybe we use this one thru 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.
I see you didn't do this, which is fine. Shall we create an issue to do this work?
Actually, maybe that issue can just request implementing the same folder+tab header design for the User Management page. In order to have a consistent design across all pages of the this application.
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.
Oh, and then separately, an issue to track the warnings / errors in Auth0Login
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.
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
Co-authored-by: Rudo Kemper <[email protected]>
rudokemper
left a comment
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.
Looks great! Please just go ahead and make the one requested change about actually adding the "Need help" blurb to the index before merging, and create an issue to track user management header redesign work.
| @@ -0,0 +1,126 @@ | |||
| <script lang="ts" setup> | |||
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 see you didn't do this, which is fine. Shall we create an issue to do this work?
Actually, maybe that issue can just request implementing the same folder+tab header design for the User Management page. In order to have a consistent design across all pages of the this application.
| "needHelp": "Need help? Visit the Guardian Connector", | ||
| "documentationWebsite": "documentation website", |
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.

Goal
Closes #19.
Screenshots
What I changed
text-xsat 1200px and below for Guardian Connector, welcome message, and sign out buttonWhat I'm not doing here
LLM use disclosure
Used LLM for consultation and code generation throughout the responsive redesign implementation, including: