-
Notifications
You must be signed in to change notification settings - Fork 1
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
popup issue fix #323
popup issue fix #323
Conversation
Reviewer's Guide by SourceryThis pull request addresses a popup issue by modifying the navigation behavior in the account selection component. The changes involve replacing Link components with custom onClick handlers that use setTimeout to delay navigation, allowing the popup to close before redirecting. Sequence diagram for popup issue fix in account selectionsequenceDiagram
actor User
participant UI as User Interface
participant Nav as Navigation
User->>UI: Click on account option
UI->>UI: setOpen(false)
UI->>UI: setTimeout(..., 200ms)
UI-->>User: Close popup
UI->>Nav: navigate to /{account}/environments
User->>UI: Click on 'Create team'
UI->>UI: setOpen(false)
UI->>UI: setTimeout(..., 200ms)
UI-->>User: Close popup
UI->>Nav: navigate to /new-team
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @tulsiojha - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
to={`/${parseName(item)}/environments`} | ||
onClick={() => { | ||
setOpen(false); | ||
setTimeout(() => { |
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.
suggestion (bug_risk): Reconsider the use of setTimeout for navigation
Using setTimeout to delay navigation could lead to unexpected behavior if the user interacts with the page during the delay. Consider using the onTransitionEnd event of the OptionList closing animation instead for more reliable navigation timing.
onTransitionEnd={() => {
navigate(`/${parseName(item)}/environments`);
}}
Summary by Sourcery
Bug Fixes: