-
Notifications
You must be signed in to change notification settings - Fork 0
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
Resolve 409 org selection component #464
Conversation
5b0d201
to
76c8aa4
Compare
@cannandev! I’m really excited to check out this PR — thanks so much for pulling it together. I just pulled everything down, and I think I might be missing some styles? Most of the UI’s there, but it’s missing some design elements that were definitely in yesterday’s local preview: Is everyone else seeing this locally, or is it just me? |
@beepdotgov Here's a screen of what I see when I pull down the branch. There might be some caching going on, either with the build or with your browser. Try restarting the dev server, or hard refreshing the page: |
Edit: Never mind! Was on the wrong branch 🤦🏻 ( |
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.
@cannandev Mentioned this in the comments, but this is looking very good. I left a few design notes, along with some non-blocking suggestions — please holler if anything’s unclear, or worth discussing further!
So exciting to see this in action! 🚀
Signed-off-by: Claire Annan <381122+cannandev@users.noreply.github.com>
Hi @echappen and @beepdotgov I'm back 👋🏾 Ethan, I think I got everything. Just two notes:
Do we want a global variable for transition timing? |
@echappen I added a super basic test using Testing Library |
That’s great, thanks @cannandev!
Oh, great idea. If we’re using 0.2s everywhere, let’s go with that! (I could see us potentially needing different timings for different kinds of animation, but we can cross that bridge when/if we get to it.) |
Tests look great to me, thanks for putting those together! |
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: Eleni Chappen <eleni.chappen@gsa.gov> Signed-off-by: Claire Annan <381122+cannandev@users.noreply.github.com>
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.
🚀
Changes proposed in this pull request:
How to review these changes locally:
{true}
.Related issues
Resolves #409
Blocks #408, #410
Needed for #518
Submitter checklist
Security considerations
Not sure if this is a security consideration, but confirmed with the engineers users will always be part of at least one organization. I did not add a state for no organization.
Design considerations
Discussed the following with Ethan, who gave a 👍🏾
use
option, so icon-size and text-color can be applied.scrollbar-width: thin
for overflow. This doesn’t work on Safari or Android browsers. Fall back to browser default.