-
Notifications
You must be signed in to change notification settings - Fork 4
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
frontend page for creating a new realm #1521
base: dev
Are you sure you want to change the base?
Conversation
@calebeby At line 37 in |
I don't totally understand what the test files are doing. I messed around with them a little bit, but I couldn't get the check to pass. I only changed the two under components. Can you take a look at those and either recommend something to do or fix it yourself? |
@varCepheid The test fails because the login form is broken. The test is intended to catch bugs like this, so the test is doing its job perfectly! If you go to the preview for this PR: https://deploy-preview-1521--peregrine.netlify.app/leaderboard Once you type a valid username and password, the "submit" button should stop being disabled, so you can click it. (That's what happens on https://dev.peregrinefrc.com/leaderboard) I am not sure exactly what you changed in your PR that caused this behavior to break, but a good starting point would be to try undoing each of your changed files one by one until it is fixed, so that you can narrow down what caused the problem. |
@varCepheid I am guessing that this change is what's breaking it: https://github.com/Pigmice2733/peregrine-frontend/pull/1521/files#diff-7f29dc5f7895e5965f3a69c6f27d41be530b11108dba3c035c8287ec491b4cf2R22 I am not sure the intended behavior of that change, but because of the change, the ref never gets passed in to the form element, so the ref never gets set, so there is no way for the component to check whether its form is valid. The way that the code was before you made a change was correct. |
Also @varCepheid once you fix the bug, if you could undo the unrelated parts of this PR that'd be great! Smaller PR's are much easier to review and merge, and they'll make it easier to track down changes in the git history once we merge them too! |
A lot of the changes that aren't connected to this issue are fixing errors that make the checks fail. Is it better to fix them in separate PRs and put this one on hold? |
@varCepheid The checks are passing on the |
@varCepheid I have a question about how this flow works for users. If a team was to create a new realm, now that realm has no users. So in order for them to start making approved users for their realm, they'd need to make an unverified user in that realm, and still go through the manual process of contacting pigmice and getting their account verified and set to admin. Then they could go and make other users. Is it intended to still have that manual step there or were you envisioning something different? |
@varCepheid it looks like newly-created realms have |
The |
I realized that making a user automatically an admin A. is a bad idea and B. gives us no oversight of new realms. I think we should just indicate that a new user in a new realm needs to join the Slack to become an admin. |
closes #1510