-
Notifications
You must be signed in to change notification settings - Fork 127
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: Add 48px margin between menu and domain list #1123
base: testnet
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@anljvnsjnfnutdfns is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request includes two styling modifications. A new blank line has been added at the beginning of Changes
Suggested labels
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)
🪧 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 (
|
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
🧹 Nitpick comments (1)
styles/components/identitiesV1.module.css (1)
1-1
: Extraneous Blank Line
A blank line has been added at the top of this CSS file. While this does not affect functionality or styling, you might consider removing it for consistency unless it's intentionally used for visual separation or to adhere to specific coding style guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
components/identities/skeletons/identitiesSkeleton.tsx
(1 hunks)styles/components/identitiesV1.module.css
(1 hunks)
🔇 Additional comments (1)
components/identities/skeletons/identitiesSkeleton.tsx (1)
7-7
: Margin Update Applied via Tailwind Class
The update to the<div>
now using the"mt-12"
class correctly applies a margin of 48px (3rem, assuming the base unit is 16px). This directly addresses the PR objective by adding the desired spacing between the menu and the domain list.
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, thanks for your PR. But the bug seems to be fixed
@anljvnsjnfnutdfns Kevin just confirmed me he still has the problem. He uses the Braavos app |
@Marchand-Nicolas okay I'll work on it again |
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
🧹 Nitpick comments (1)
styles/components/navbar.module.css (1)
66-66
: Margin Value Verification:
The added CSS propertymargin-bottom: 2rem;
in the.navbarContainer
class is intended to create spacing between the menu and the domain list. However, please verify that2rem
indeed corresponds to the intended 48px margin. If your base font-size is 16px, then 2rem equals 32px—which would fall short of the 48px target. Consider using3rem
(if 1rem is 16px) or confirming that the base font-size is set such that 2rem equals 48px.
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.
@anljvnsjnfnutdfns, avoid using margin-bottom for spacing. Since the navbar has position: fixed, it remains on an upper layer, causing it to shadow the domain list.
Instead, increase the margin-top of the domain list container to create the necessary spacing.
Okay. I will work on it
…On Thu, 13 Mar 2025, 11:30 pm EDOHWARES, ***@***.***> wrote:
***@***.**** commented on this pull request.
@anljvnsjnfnutdfns <https://github.com/anljvnsjnfnutdfns>, do not use
margin bottom for the spacing(as the navbar has a position of fixed:
meaning its at an upper layer, thereby causing it to shadow the domain
lists).
Increase the margin-top of the domain lists container instead.
—
Reply to this email directly, view it on GitHub
<#1123 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBML6Q5QUIHOGFAVQJGWGID2UIBKFAVCNFSM6AAAAABYTYAX36VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMOBTGQ3DKOBXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
@@ -4,7 +4,7 @@ import styles from "../../../styles/components/identitiesV1.module.css"; | |||
|
|||
const IdentitiesSkeleton: FunctionComponent = () => { | |||
return ( | |||
<div className="mt-12"> | |||
<div className=""> |
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.
Do not keep empty classe names
@Marchand-Nicolas can you pls review the changes |
@anljvnsjnfnutdfns I have already reviewed it. Please check my review |
close: #1045
Summary by CodeRabbit