Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(ad-hoc): Add create team dialog #8846
feat(ad-hoc): Add create team dialog #8846
Changes from 64 commits
e30c7a6
d9686bb
4e8958d
a4c51b1
a3d5b02
5a5bc42
a8bc994
ea045d7
b0aa11f
31fe645
092ab6f
83f7d02
192056f
7d99836
c6feaeb
3082e9e
291adf2
89f16d5
3d2f558
53e6a8f
f8a2d00
ac3e7ec
fb1d7b5
69c68b7
0cee2c3
4de5d3d
39ce1d2
fe0770d
8df1d81
f45594f
8b3a4ba
1ec5798
8504616
081e9b0
6f7cf37
2517335
ff7dcf6
f344dbd
81cd0a9
85b9bfe
4623603
dffeb64
083843f
7c1c947
0023196
550d10e
55c3de3
474cf5e
fb9e064
235e1b8
072a26c
2e87931
9797318
5fc1f9f
f8e238d
168db2e
8f9cd40
3646768
72fbd1c
6b7b44c
1293118
254b623
bc6d823
46525c6
47566e6
0254bad
27f506e
399f91c
2b804e0
6da1745
cc91ab8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
+1 it looks like the limit in pg is 100 characters so I was wondering why 50 here?
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.
@nickoferrall we have 50 characters validation set on the backend and when we creating a team from http://localhost:3000/newteam/1. So I kept it as is here too
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.
+1 I think the org picker should always be visible so the height of the modal doesn't change when you select an item. It would also be reassuring for me that I've selected the right team from the right org
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.
If org picker will be always visible it will show one single org in most of cases. Would this be ok? Tagging @acressall for this design question
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 actually don't mind the small height change. I suspect that a lot of users aren't aware of the concept of orgs, and it could be especially confusing in this context, so I only want to bring it up when we actually need the information
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.
-1 we're enforcing a team name length limit of 50 characters, but not giving users feedback that that will happen. So if I create a team with 55 characters, the last 5 characters get removed without me knowing.
We could add
maxLength={MAX_TEAM_NAME_LENGTH}
to the input. Or, for the optimal UX, we could give the user some feedback that there's a limit.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 just removed this now, and allow backend to validate this. I think rarely someone would type 50 chars
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.
+1 I'd prefer the buttons to be a bit larger as they feel slightly small to me:
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.
made bigger
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.
+1 normally one would mount the dialog and then only modify
isOpen
so we can get a transition.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.
Transition works for me like this. Can you record a video?
I'll merge this PR in the mean time, and will fix it in next PRs if it is an issue
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.
-1 on focus, this new input is blue, but the rest of our inputs go from grey to black. Also, the font is bolder in this input vs others.
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 made a new input to be matched with new select and multi select components we introduced previously. Not sure if I need to change it to match our old components, tagging @acressall for this question
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.
The design team has been working add focus states, and they are all sky 500, so this is good!
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 think the weight is incorrect though, in the designs the label is semibold, but the placeholder and input are regular
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.
Updated: