Skip to content
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

feat(ui): improve admin setup ux #1671

Merged
merged 11 commits into from
Mar 21, 2024
Merged

feat(ui): improve admin setup ux #1671

merged 11 commits into from
Mar 21, 2024

Conversation

wwayne
Copy link
Contributor

@wwayne wwayne commented Mar 14, 2024

@wwayne wwayne requested review from wsxiaoys and liangfung March 14, 2024 05:27
@wsxiaoys
Copy link
Member

wsxiaoys commented Mar 14, 2024

Looks nice to me. Add @liangfung for review from implementation perspective. Will review the text once it's done.

@wsxiaoys wsxiaoys removed their request for review March 14, 2024 07:39
@liangfung
Copy link
Collaborator

LGTM. By the way, do we need to add a redirect logic so that once the server has been initialized by an admin, it redirects to the signup page. Otherwise, it will follow the regular user signup logic.

@wwayne wwayne marked this pull request as ready for review March 15, 2024 08:50
@wsxiaoys wsxiaoys marked this pull request as draft March 18, 2024 00:55
@wwayne
Copy link
Contributor Author

wwayne commented Mar 18, 2024

  • After server intialization(admin already finished setup), going to the /auth/signup?isAdmin=true will be redirected to /auth/signin related codes
  • Add a new component admin-register-step for extracting Step component
  • Instead of removing the scrollbar, I update CSS to make sure the scrollbar won't show and hide as before, video record: https://jam.dev/c/aa5730b9-a25e-4adb-b045-5da2f16a881c

@wwayne wwayne marked this pull request as ready for review March 18, 2024 03:38
Copy link
Member

@wsxiaoys wsxiaoys left a comment

Choose a reason for hiding this comment

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

Instead of removing the scrollbar, I update CSS to make sure the scrollbar won't show and hide as before, video record: https://jam.dev/c/aa5730b9-a25e-4adb-b045-5da2f16a881c

Please disable scroll.

@@ -0,0 +1,23 @@
import { cn } from '@/lib/utils'

export default function AdminRegisterStep({
Copy link
Member

Choose a reason for hiding this comment

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

nit: since this is only used in admin-register, it's ok to put it inside of that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

this file generates following warning

<w> [webpack.cache.PackFileCacheStrategy] Skipped not serializable cache item 'Compilation/modules|/Users/meng/Projects/tabby/ee/tabby-ui/node_modules/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[13].oneOf[12].use[2]!/Users/meng/Projects/tabby/ee/tabby-ui/node_modules/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[13].oneOf[12].use[3]!/Users/meng/Projects/tabby/ee/tabby-ui/app/auth/signup/components/admin-register.css': No serializer registered for Warning
<w> while serializing webpack/lib/cache/PackFileCacheStrategy.PackContentItems -> webpack/lib/NormalModule -> Array { 1 items } -> webpack/lib/ModuleWarning -> Warning


Copy link
Contributor Author

@wwayne wwayne Mar 18, 2024

Choose a reason for hiding this comment

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

change .xxx{ &:after {} } to .xxx:after {} removed this warning

@wsxiaoys wsxiaoys marked this pull request as draft March 18, 2024 03:44
@wsxiaoys
Copy link
Member

Please also make UI components non-interactive when it's not active:

https://jam.dev/c/123358e6-d4ce-48ab-8ba5-264f77981c9f
(i can select / interact with input form, even though it's not the current step)

@wwayne
Copy link
Contributor Author

wwayne commented Mar 18, 2024

Updated

  • disable scrolling
  • hidden component should not be interactive

https://jam.dev/c/c9583fa5-6871-41f7-a2d3-306bb638db10

@wwayne
Copy link
Contributor Author

wwayne commented Mar 18, 2024

btw I found the input got warrning component is changing an uncontrolled input to be controlled....
I found the solution in here: shadcn-ui/ui#410
so I added their default values

const form = useForm<z.infer<typeof formSchema>>({
    resolver: zodResolver(formSchema),
    defaultValues: {
      email: "",
      password1: "",
      password2: "",
      invitationCode
    }
  })

@liangfung can you help me to double check if it would break anything? thanks

I checked /signin /signup?invitationCode=xxx , and looks all good

@wwayne wwayne marked this pull request as ready for review March 18, 2024 05:52
@liangfung
Copy link
Collaborator

btw I found the input got warrning component is changing an uncontrolled input to be controlled.... I found the solution in here: shadcn-ui/ui#410 so I added their default values

Nice, with this change the functionality works normally, and it can remove the warnings. However, it might cause the form to lose the required error messages, and as more fields are added, each might need to be assigned individually. Of course, the server will validate the form submission parameters.
cc @wsxiaoys

before:
image

after:
image

@liangfung
Copy link
Collaborator

shadcn-ui/ui#410 (comment)
This solution is also good; there's no need to set empty string defaultValues, and it can still trigger required_error. However, it needs to be set in each input.

@wwayne
Copy link
Contributor Author

wwayne commented Mar 18, 2024

Thanks @liangfung , I switched to the new solution
It can dislay required warning now

@wwayne
Copy link
Contributor Author

wwayne commented Mar 19, 2024

image
This issue fixed as well

@wsxiaoys wsxiaoys enabled auto-merge (squash) March 21, 2024 06:51
@wsxiaoys wsxiaoys merged commit 47fd912 into main Mar 21, 2024
4 checks passed
@wsxiaoys wsxiaoys deleted the admin-setup-ux branch March 21, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants