-
Notifications
You must be signed in to change notification settings - Fork 15
feat(namehash-ui): refine package #1551
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d148d5a The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request consolidates mobile detection by migrating a local Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Pull request overview
This PR refines the namehash-ui package by improving reusability and reducing code duplication. The changes consolidate a duplicated mobile detection hook and make the RegistrarActionCard component more flexible by parameterizing the referrer link function.
Changes:
- Enhanced
useIsMobilehook with configurable breakpoint parameter and exported it from the package - Refactored
RegistrarActionCardto accept referrer link function as a parameter instead of hardcoding it - Replaced duplicate
useIsMobileimplementation in ensadmin app with the shared hook from namehash-ui
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/namehash-ui/src/index.ts | Exports useIsMobile hook and all chain utilities (previously only getChainName) |
| packages/namehash-ui/src/hooks/useIsMobile.tsx | Added optional breakpoint parameter with default value for configurability |
| packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | Parameterized referrer link function and exported LabeledField component |
| apps/ensadmin/src/hooks/use-mobile.tsx | Removed duplicate implementation in favor of shared hook |
| apps/ensadmin/src/components/ui/sidebar.tsx | Uses shared useIsMobile hook with custom 768px breakpoint |
| apps/ensadmin/src/components/registrar-actions/display-registrar-actions-panel.tsx | Passes getEnsManagerAddressDetailsUrl as referrer link function parameter |
| .changeset/fruity-numbers-fry.md | Added changeset documenting the patch version changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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: 2
🤖 Fix all issues with AI agents
In @.changeset/fruity-numbers-fry.md:
- Around line 1-5: The MD041 warning is triggered by YAML front matter in
.changeset/fruity-numbers-fry.md; update the repo lint config so markdownlint
allows front matter in changeset files (e.g., add a markdownlint config rule to
exclude .changeset/** or set MD041: false for that glob) or adjust the linter
task to skip the .changeset directory so the YAML front matter is permitted and
the lint job no longer fails.
In
`@packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx`:
- Around line 60-67: The prop getReferrerLink on
ResolveAndDisplayReferrerIdentityProps was made required and breaks consumers;
make getReferrerLink optional and provide a backward-compatible default by
updating the type ReferrerLinkFunction to be used as getReferrerLink?:
ReferrerLinkFunction and in the RegistrarActionCard (or the component that
consumes ResolveAndDisplayReferrerIdentityProps) default undefined to the
existing helper getEnsManagerAddressDetailsUrl when not supplied; update places
that construct ResolveAndDisplayReferrerIdentityProps to allow omission of
getReferrerLink and ensure the runtime uses getEnsManagerAddressDetailsUrl for
null/undefined values.
| --- | ||
| "@namehash/namehash-ui": patch | ||
| --- | ||
|
|
||
| Refines several features as a result of the application of the package in ENSAwards. |
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.
markdownlint MD041 warning on front matter.
If markdownlint runs on .changeset/**, configure it to allow YAML front matter or disable MD041 for changesets to avoid lint failures.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
In @.changeset/fruity-numbers-fry.md around lines 1 - 5, The MD041 warning is
triggered by YAML front matter in .changeset/fruity-numbers-fry.md; update the
repo lint config so markdownlint allows front matter in changeset files (e.g.,
add a markdownlint config rule to exclude .changeset/** or set MD041: false for
that glob) or adjust the linter task to skip the .changeset directory so the
YAML front matter is permitted and the lint job no longer fails.
| type ReferrerLinkFunction = (address: Address, namespaceId: ENSNamespaceId) => URL | null; | ||
|
|
||
| interface ResolveAndDisplayReferrerIdentityProps | ||
| extends Omit<ResolveAndDisplayIdentityProps, "identity" | "identityLinkDetails"> { | ||
| chainId: DefaultableChainId; | ||
| referral: RegistrarActionReferral; | ||
| getReferrerLink: ReferrerLinkFunction; | ||
| } |
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.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the relevant file
find . -type f -name "RegistrarActionCard.tsx" | head -5Repository: namehash/ensnode
Length of output: 139
🏁 Script executed:
# Read the file to check the current state
cat -n ./packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | head -100Repository: namehash/ensnode
Length of output: 3885
🏁 Script executed:
# Continue reading to see the RegistrarActionCard interface and props
cat -n ./packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | sed -n '100,250p'Repository: namehash/ensnode
Length of output: 7589
🏁 Script executed:
# Continue reading to see how getReferrerLink is used
cat -n ./packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | sed -n '250,350p'Repository: namehash/ensnode
Length of output: 5065
🏁 Script executed:
# Continue reading to see getReferrerLink usage
cat -n ./packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | sed -n '350,400p'Repository: namehash/ensnode
Length of output: 1058
🏁 Script executed:
# Search for all usages of RegistrarActionCard
rg -n "RegistrarActionCard" --type tsx | head -50Repository: namehash/ensnode
Length of output: 87
🏁 Script executed:
# Search for all usages of RegistrarActionCard with glob pattern
rg -n "RegistrarActionCard" -g '*.tsx' -g '*.ts'Repository: namehash/ensnode
Length of output: 1565
🏁 Script executed:
# Check the usage in display-registrar-actions-panel.tsx
cat -n ./apps/ensadmin/src/components/registrar-actions/display-registrar-actions-panel.tsx | sed -n '1,100p'Repository: namehash/ensnode
Length of output: 3784
Breaking API: referrerLinkFunction is now required.
This is a public component prop change and will break existing consumers (especially with a patch changeset). Consider making it optional with a backward-compatible default (e.g., getEnsManagerAddressDetailsUrl), or bump major + document migration.
💡 Suggested compatibility-friendly default
-import { getBlockExplorerTransactionDetailsUrl } from "../../utils/blockExplorers";
+import { getBlockExplorerTransactionDetailsUrl } from "../../utils/blockExplorers";
+import { getEnsManagerAddressDetailsUrl } from "../../utils/ensManager";
@@
export interface RegistrarActionCardProps {
@@
links: {
name: IdentityLinkDetails;
registrant: IdentityLinkDetails;
- referrerLinkFunction: ReferrerLinkFunction;
+ referrerLinkFunction?: ReferrerLinkFunction;
};
@@
export function RegistrarActionCard({
@@
}: RegistrarActionCardProps) {
const isMobile = useIsMobile();
+ const referrerLinkFunction =
+ links.referrerLinkFunction ?? getEnsManagerAddressDetailsUrl;
@@
- getReferrerLink={links.referrerLinkFunction}
+ getReferrerLink={referrerLinkFunction}
@@
- getReferrerLink={links.referrerLinkFunction}
+ getReferrerLink={referrerLinkFunction}🤖 Prompt for AI Agents
In
`@packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx`
around lines 60 - 67, The prop getReferrerLink on
ResolveAndDisplayReferrerIdentityProps was made required and breaks consumers;
make getReferrerLink optional and provide a backward-compatible default by
updating the type ReferrerLinkFunction to be used as getReferrerLink?:
ReferrerLinkFunction and in the RegistrarActionCard (or the component that
consumes ResolveAndDisplayReferrerIdentityProps) default undefined to the
existing helper getEnsManagerAddressDetailsUrl when not supplied; update places
that construct ResolveAndDisplayReferrerIdentityProps to allow omission of
getReferrerLink and ensure the runtime uses getEnsManagerAddressDetailsUrl for
null/undefined values.
Lite PR → WIP (will update the description once it's ready for review)
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)
PR Creation Tips
Summary by CodeRabbit
New Features
Refactor
API Improvements
✏️ Tip: You can customize this high-level summary in your review settings.