-
Notifications
You must be signed in to change notification settings - Fork 78
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
allow a player to store their pronouns as part of their profile #711
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/metafam/the-game/Ap5VQqU1Zfxp5vmqypjVS8CvaSrd [Deployment for 15c58c5 failed] |
@jaburx Thank you so much for working on this. Could you take a look at the build issues ? Seems like typecheck is failing. Also would be super great if you split this PR into two where the first one includes only backend changes, and while the second one include only frontend changes. This would greatly help in the code review & merge process, and also that way the frontend only PR can have a working preview deployment to test with :) Cheers! |
@@ -7,7 +7,7 @@ import React, { useState } from 'react'; | |||
|
|||
export const SetupDone: React.FC = () => { | |||
const router = useRouter(); | |||
const { user } = useUser({ redirectTo: '/' }); |
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.
¿Could you describe why you removed this?
I always thought it was a little semantically stilted, but didn't change it b/c of not wanting to introduce side effects I was unaware of.
Maybe to guarantee a continuance of behavior you could set a default value in the useUser
definition of redirectTo = '/'
.
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 was causing the buggy behaviour when visiting one of the profile setup pages and the user would just get redirected to /players, i fixed it in packages/web/lib/hooks/index.ts
by renaming the redirectIfFound
into redirectIfNotFound
and setting to false
by default and tweaking some of its behaviour. Now everything works the same, apart from the bug in player setup pages, which is gone.
¿@jaburx, have you abandoned this code? You're pretty close. If you respond to a couple of my comments, I'll merge this. |
Is this issue still open? Can i look into this? @dysbulic |
6be80eb
to
f2552ab
Compare
c0a8b88
to
b5601fe
Compare
Closing to trigger the Google Cloud builds. |
So, @vidvidvid, the Google deployments are successful, so everything’s good. I'll review it shortly. |
nicu 👍 |
This code is based on a version of the profile editing page that doesn't exist anymore. This is in part a result of the time it took to complete the review, which is largely because I was unaware the issues I had raised had been resolved. I've been trying to figure out how to remove Dependabot from my GitHub activity feed. I turned off notifications in the settings, but I keep getting them. They make it harder to distinguish important updates. Also, however, the redesign of the profile pages has been ongoing for a almost a month now, so this issue shouldn't be a surprise to anyone. To me, the correct next step is to put in a request to the design guild for a profile page with pronouns in it. ¿Right, @davort? |
Hey, the setup wizard steps still exist. They are just accessible only the first time the user logs in and sets up their profile. |
@dysbulic i'm gonna merge the develop branch into it and fix the conflicting files, ok? |
actually, i would also need to add the pronouns input field to the new profile edit dialog, but this can be done after this pr is merged, so we don't keep bloating it up. what do you think? |
For some reason the build appears to fail, but when I run |
@vidvidvid, just one last quibble. On the roles page which precedes pronouns, the submit button is entitled "Professional Profile" rather than "Pronouns". Also, on the roles page, could you switch |
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.
Overall looks good. I had some syntactic suggestions, but the only real blocking issue is the text of the submit button on the preceding page.
Two requests, now that the edit profile branch is in here:
|
I don't think pronouns as a separate/standalone step in the onboarding flow is optimal. In principle, we'd want to keep the whole flow as fluid as possible. That said, perhaps we ought to consider "bundling" the choice of pronouns with some other, simpler choices, such as "Display name" or "Website"? |
WIll take care of @dysbulic 's and @alalonde 's suggestions tomorrow, gonna sleep now. @davort I think your suggestion makes sense and it would be nice to bundle up some of the choices, but I'd prefer to get done with this pr and make another issue for a slight re-work of the setup wizard - that way 1. you get time to make a plan on what to put together and 2. we can finally close this pr that's been open for months, what do you think? |
Thanks @vidvidvid - sounds good to me. Let's do like you suggested 👍 |
245edca
to
15c58c5
Compare
Fixed @dysbulic 's suggestions, also slightly improved the buttons from the screenshot you provided: Also took care of @alalonde 's requests:
We can eventually make a follow up with re-organisation of the setup wizard steps. |
I would go even farther and say that it shouldn't be in the onboarding flow at all. There are probably too many steps already; really only the most critical information should be required during onboarding. And I suspect that the people that really feel strongly about inputting their pronoun preferences will easy find the field in the edit profile modal. |
Successfully undeployed the Preview of this Pull Request |
Adds pronouns as a profile characteristic.
Paired With: @vidvidvid