fix: Workspace Permissions overlaps with their descriptions on smaller windows #2127
Conversation
harshsbhat
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
308-308: Improved responsive layout for permissions.The modification to the width classes effectively addresses the PR's main objective of fixing the overlap issue on smaller windows. The responsive approach ensures that the layout works well across different screen sizes:
- Wider layout (4/6) on smaller screens prevents overlap.
- Added right margin (mr-2) provides better spacing.
- Original width (1/3) is maintained for medium and larger screens.
This change should resolve the issue described in #2125.
Consider adding a small breakpoint-specific class for extra small screens if needed, e.g.,
xs:w-full. This would ensure optimal layout on very small devices. Example:- <div className="w-4/6 mr-2 md:w-1/3"> + <div className="w-4/6 mr-2 md:w-1/3 xs:w-full">
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (2 hunks)
Additional comments not posted (2)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (2)
198-198: Improved description text clarity.The modification in the description text enhances readability without changing the meaning. This change aligns well with the PR's objective of addressing visual issues, as it may help reduce text length and potential overlap in the UI.
Line range hint
1-329: Overall assessment: Changes effectively address the UI issue.The modifications in this file successfully tackle the visual bug described in issue #2125. The combination of text simplification and responsive layout adjustments should prevent permission names from overlapping with their descriptions on smaller windows. The changes are well-implemented and align perfectly with the PR objectives.
Some additional points to consider:
- Test the changes on various screen sizes to ensure the layout behaves as expected.
- Consider adding unit or integration tests to prevent regression of this UI issue in the future.
- Update any relevant documentation or UI/UX guidelines to reflect these changes.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
|
it was happening for just one permission. Breaking the permission name didn't look good either. w-full looks decent I have pushed those changes |
|
@chronark what do you think about having the permission description flow underneath the permission name at smaller screens? |
|
yes, only for small screens also I think there's a max width or something we need to remove on small screens, there's plenty of space, yet we have a linebreak here |
|
yes, I just quickly added flex-col to see how it looks. I will push in a few minutes |
|
Latest changes @chronark |
|
what if we align the text? wdyt? |
|
LGTM. wdyt? Should I push this? |
|
yes please |
done |
I accidentally removed it and didn't realise it as the changes in ui were very minimal. I will do the changes and push asap |
|
@chronark should work now |
|
|
|
that's weird. I checked at least a few times. |
|
It's probably because I was referring to the live app. Which also has this misalignment. |
wdym? it's aligned in prod |
|
The SS you shared is for smaller screens. I think this PR is ready for review. BIGGER SCREENS:
SMALLER SCREENS:
|
|
Yes the previous screenshot was on a smaller screen, but it's also correctly aligned on large screens in prod |
|
I never said this explicitely, but I mean center-aligned, the |
it's required for a PC ( it makes just a little difference) |
|
I'm sorry, what's |
ah, I meant longer screen ( PC screen ). My bad should have been more clear about it |
2 similar comments
ah, I meant longer screen ( PC screen ). My bad should have been more clear about it |
ah, I meant longer screen ( PC screen ). My bad should have been more clear about it |
What does this PR do?
Fixes #2125
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Style