-
Notifications
You must be signed in to change notification settings - Fork 509
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
fix: Create key to be sticky #2435
base: main
Are you sure you want to change the base?
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@Roshansuthar1105 is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Thank you for following the naming conventions for pull request titles! 🙏 |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
Line range hint
280-289
: LGTM: Improved state management on dialog close.The modifications to the
onOpenChange
handler effectively reset the component state when the dialog is closed, which is a good practice for maintaining a clean state. The navigation to "/settings/root-keys" after closing the dialog provides a smooth user experience.For consistency, consider using the
setName
function to reset the name state, similar to howsetSelectedPermissions
is used:if (!v) { // Remove the key from memory when closing the modal key.reset(); setSelectedPermissions([]); - setName(""); + setName(undefined); router.push("/settings/root-keys"); }This ensures that the
name
state is reset to its initial value ofundefined
as defined in theuseState
hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (2)
263-263
: LGTM: Button positioning aligns with PR objectives.The addition of the
sticky bottom-0
class to the Button component successfully implements the sticky positioning at the bottom of the page, as outlined in the PR objectives. This change enhances user experience by ensuring the button remains visible during scrolling, improving accessibility for users interacting with the key creation functionality.
Line range hint
1-391
: Overall assessment: Changes effectively implement PR objectives.The modifications in this file successfully address the PR objectives by implementing a sticky button component and improving state management. The changes enhance user experience and maintain good coding practices. The implementation aligns well with the intended functionality and contributes positively to the overall quality of the codebase.
Key points:
- Sticky button positioning improves accessibility and user interaction.
- Enhanced state management on dialog close ensures a clean component state.
- The changes are focused and do not introduce unnecessary complexity.
Great job on these improvements!
Please sign the CLA @Roshansuthar1105 |
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.
See my previous note.
What does this PR do?
This pull request implements a button component with a sticky position at the bottom of the page. The button triggers a mutation to create a new key based on the provided name and selected permissions. The changes address user experience by ensuring the button remains visible during scrolling, enhancing usability.
No specific issue is linked to this change, but it improves accessibility for users interacting with the key creation functionality. There are no additional dependencies required for this update.
Fixes #2414
This pull request does not currently address a specific issue. However, it aims to enhance the user experience by ensuring that the button for creating a new key remains accessible at the bottom of the page during scrolling.
Type of change
How should this be tested?
Checklist
Required
pnpm build
pnpm fmt
console.logs
git pull origin main
Appreciated
Summary by CodeRabbit
New Features
Bug Fixes
Performance Improvements