feat: [SC-24987] create LoadingReportIcon #282
Conversation
FrancoAguzzi
requested review from
djstrong,
Carbon225,
notrab,
lightwalker-eth,
BanaSeba and
eduramme
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: bc4192d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
There was a problem hiding this comment.
@FrancoAguzzi Hey great to see the progress here. Reviewed and shared feedback 👍
packages/nameguard-react/src/components/LoadingReportIcon/LoadingReportIcon.tsx
Outdated
Show resolved
Hide resolved
packages/nameguard-react/src/components/LoadingReportIcon/LoadingReportIcon.tsx
Outdated
Show resolved
Hide resolved
packages/nameguard-react/src/components/LoadingReportIcon/LoadingReportIcon.tsx
Outdated
Show resolved
Hide resolved
packages/nameguard-react/src/components/LoadingReportIcon/LoadingReportIcon.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@FrancoAguzzi @notrab Thanks for updates. Reviewed and shared feedback 👍
| window.location.href = `https://nameguard.io/inspect/${encodeURIComponent( | ||
| ensName.name, | ||
| )}`; | ||
| const onClickHandler = (clickHandlerFor: ClickHandlerFor) => { |
There was a problem hiding this comment.
Note to self: If time allows I want to look at this closer.
| window.location.href = `https://nameguard.io/inspect/${encodeURIComponent( | ||
| ensName.name, | ||
| )}`; | ||
| const onClickHandler = (clickHandlerFor: ClickHandlerFor) => { |
There was a problem hiding this comment.
Note to self: if time allows have a closer look at this.
There was a problem hiding this comment.
@FrancoAguzzi Thanks for updates. Reviewed and shared feedback
| @@ -3,7 +3,10 @@ import { defineConfig } from "tsup"; | |||
| export default defineConfig({ | |||
| entry: ["src/index.ts"], | |||
| format: ["esm"], | |||
| splitting: true, | |||
| sourcemap: true, | |||
| minify: false, | |||
There was a problem hiding this comment.
Any changes to these tsup.config.js files either need to be reverted or discussed and aligned with @notrab. Thanks
There was a problem hiding this comment.
@FrancoAguzzi Whenever you make any changes to any build configuration files, please proactively add a comment about that change inside the PR in GitHub that explains:
- Why that change was needed.
- The current status of getting approval from @notrab of the change.
| onClickOverride, | ||
| onIconClickOverride, | ||
| onBadgeClickOverride, | ||
| onTooltipClickOverride, |
There was a problem hiding this comment.
@FrancoAguzzi I don't think we're going in the correct direction with this PR. We're not following our principles for composability.
- Let's please avoid
ReportBadgebecoming another "god" UI component with too many props. We need to design our UI components as needed so that they are composable. - Why do we need all of these different "on click override" functions on each UI component? Why can't the code that wants to make use of
ReportBadgejust pass in some slots / props as needed? For example: There can be a slot for theiconin aReportBadge. If someone wants the icon to have a special click override, they can pass special code into that slot as might be desired. - I'm not sure we want so many degrees of freedom with so many customizations. It's best to keep it simple. Perhaps we remove some of these customizations entirely unless we see some real need for them right now.
| onClickOverride?: (ensName: ENSName) => void; | ||
| type ReportShieldProps = { | ||
| onIconClickOverride?: (ensName: ENSName) => void; | ||
| onTooltipClickOverride?: (ensName: ENSName) => void; |
There was a problem hiding this comment.
Please see my other feedback about wanting to find a more composable and simplified solution that allows us to remove all these on click override callbacks.
| @@ -17,11 +17,12 @@ | |||
| ], | |||
| "scripts": { | |||
| "build": "tsup", | |||
| "dev": "tsup --watch --clean=false" | |||
| "dev": "tsup --watch --clean=false", | |||
| "test": "vitest" | |||
There was a problem hiding this comment.
Can you help me understand the goal for adding the test script here and not running tests from the root directory of the repo? For example, is this different than what we're doing for other packages? If so, why?
I can understand that perhaps people who get our packages via NPM aren't getting the whole monorepo and therefore maybe this has a purpose to include in each package? But in my mind, when you get a package via NPM, it shouldn't be something you're running tests against. For example we shouldn't even include the test code in the files included in the NPM package distribution. The NPM package should just contain the "main" code and no tests. Right?
Assuming we should remove this vitest script here, then please also remove vitest as a devDependency
| }, | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "types": "./dist/index.d.mts", |
There was a problem hiding this comment.
Can you please help to explain this change to .mts?
There was a problem hiding this comment.
@FrancoAguzzi Whenever you make any changes to any build configuration files, please proactively add a comment about that change inside the PR in GitHub that explains:
- Why that change was needed.
- The current status of getting approval from @notrab of the change.
packages/ens-utils/.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
|
|
|||
| # dependencies | |||
| /node_modules | |||
There was a problem hiding this comment.
Why are we making this change? This suggests we aren't running our pnpm commands from the root of the monorepo. I don't see why we should do this?
|
|
||
| type ShareProps = { | ||
| name?: string; | ||
| name: string; |
There was a problem hiding this comment.
We should make this an ENSName rather than a string.
|
|
||
| type ShareProps = { | ||
| name?: string; | ||
| name: string; | ||
| }; | ||
|
|
||
| function createTwitterLink(name: string) { |
There was a problem hiding this comment.
All of these functions should take an ENSName rather than a string.
| @@ -137,12 +136,10 @@ export function Share({ name }: ShareProps) { | |||
|
|
|||
| <div className="min-h-[200px] flex items-center justify-center px-6 md:px-10"> | |||
| {name && ( | |||
There was a problem hiding this comment.
Why are we checking here if name is defined? It should always be defined based on the defined props of this UI component.
* Fix PR 282 * Update sharing and report URL generation * Many additional enhancements * Misc small fixes * Apply suggested update
Refine Report Loading and Error States
Open tasks: