feat: [SC-24987] create LoadingReportIcon by FrancoAguzzi · Pull Request #282 · namehash/namekit

FrancoAguzzi · 2024-06-01T19:39:44Z

Open tasks:

vercel · 2024-06-01T19:39:46Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples.nameguard.io	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2024 6:40pm
nameguard.io	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2024 6:40pm
namehashlabs.org	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2024 6:40pm
storybook.namekit.io	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2024 6:40pm

changeset-bot · 2024-06-01T19:39:47Z

🦋 Changeset detected

Latest commit: bc4192d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@namehash/nameguard-react	Minor
@namehash/nameguard	Minor
@namehash/nameguard-js	Patch

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

lightwalker-eth

@FrancoAguzzi Hey great to see the progress here. Reviewed and shared feedback 👍

lightwalker-eth

@FrancoAguzzi @notrab Thanks for updates. Reviewed and shared feedback 👍

lightwalker-eth · 2024-06-06T12:39:28Z

-      window.location.href = `https://nameguard.io/inspect/${encodeURIComponent(
-        ensName.name,
-      )}`;
+  const onClickHandler = (clickHandlerFor: ClickHandlerFor) => {


Note to self: If time allows I want to look at this closer.

lightwalker-eth · 2024-06-06T12:41:53Z

-      window.location.href = `https://nameguard.io/inspect/${encodeURIComponent(
-        ensName.name,
-      )}`;
+  const onClickHandler = (clickHandlerFor: ClickHandlerFor) => {


Note to self: if time allows have a closer look at this.

lightwalker-eth

@FrancoAguzzi Nice updates! Reviewed and shared feedback 👍

… React package

…eport-icon

FrancoAguzzi · 2024-08-27T18:59:28Z

@notrab @djstrong can you review this test please?

https://github.com/namehash/namekit/actions/runs/10584114051/job/29327608472?pr=282

…eport-icon

…rt method

lightwalker-eth

@FrancoAguzzi Thanks for updates. Reviewed and shared feedback

lightwalker-eth · 2024-08-28T00:36:05Z

  format: ["esm"],
+  splitting: true,
+  sourcemap: true,
+  minify: false,


Any changes to these tsup.config.js files either need to be reverted or discussed and aligned with @notrab. Thanks

@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.

lightwalker-eth · 2024-08-28T00:46:31Z

-  onClickOverride,
+  onIconClickOverride,
+  onBadgeClickOverride,
+  onTooltipClickOverride,


@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 ReportBadge becoming 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 ReportBadge just pass in some slots / props as needed? For example: There can be a slot for the icon in a ReportBadge. 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.

lightwalker-eth · 2024-08-28T00:53:23Z

-  onClickOverride?: (ensName: ENSName) => void;
+type ReportShieldProps = {
+  onIconClickOverride?: (ensName: ENSName) => void;
+  onTooltipClickOverride?: (ensName: ENSName) => void;


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.

lightwalker-eth · 2024-08-28T00:55:35Z

    "build": "tsup",
-    "dev": "tsup --watch --clean=false"
+    "dev": "tsup --watch --clean=false",
+    "test": "vitest"


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

lightwalker-eth · 2024-08-28T00:56:59Z

  "exports": {
    ".": {
-      "types": "./dist/index.d.ts",
+      "types": "./dist/index.d.mts",


Can you please help to explain this change to .mts?

lightwalker-eth · 2024-09-11T11:36:41Z

@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.

lightwalker-eth · 2024-09-11T11:42:24Z

@@ -0,0 +1,3 @@
+
+# dependencies
+/node_modules


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?

lightwalker-eth · 2024-09-11T11:45:32Z


 type ShareProps = {
-  name?: string;
+  name: string;


We should make this an ENSName rather than a string.

lightwalker-eth · 2024-09-11T11:46:52Z

+  name: string;
 };

 function createTwitterLink(name: string) {


All of these functions should take an ENSName rather than a string.

lightwalker-eth · 2024-09-11T11:48:25Z

@@ -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 && (


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

FrancoAguzzi requested review from BanaSeba, Carbon225, djstrong, edulennert, lightwalker-eth and notrab as code owners June 1, 2024 19:39

vercel Bot deployed to Preview – examples.nameguard.io June 1, 2024 19:40 View deployment

vercel Bot deployed to Preview – nameguard.io June 1, 2024 19:40 View deployment

vercel Bot deployed to Preview – storybook.namekit.io June 1, 2024 19:41 View deployment

lightwalker-eth suggested changes Jun 4, 2024

View reviewed changes

vercel Bot deployed to Preview – examples.nameguard.io June 5, 2024 12:59 View deployment

vercel Bot deployed to Preview – storybook.namekit.io June 5, 2024 12:59 View deployment

vercel Bot deployed to Preview – nameguard.io June 5, 2024 13:01 View deployment

notrab reviewed Jun 5, 2024

View reviewed changes

Comment thread packages/nameguard-sdk/src/utils.ts Outdated

vercel Bot deployed to Preview – storybook.namekit.io June 5, 2024 20:24 View deployment

vercel Bot deployed to Preview – examples.nameguard.io June 5, 2024 20:24 View deployment

vercel Bot deployed to Preview – nameguard.io June 5, 2024 20:26 View deployment

vercel Bot deployed to Preview – nameguard.io June 5, 2024 20:52 View deployment

vercel Bot deployed to Preview – examples.nameguard.io June 5, 2024 20:53 View deployment

vercel Bot deployed to Preview – storybook.namekit.io June 5, 2024 20:54 View deployment

lightwalker-eth suggested changes Jun 6, 2024

View reviewed changes

vercel Bot deployed to Preview – nameguard.io June 6, 2024 17:34 View deployment

vercel Bot deployed to Preview – storybook.namekit.io June 6, 2024 17:34 View deployment

vercel Bot deployed to Preview – examples.nameguard.io June 6, 2024 17:35 View deployment

lightwalker-eth suggested changes Jun 6, 2024

View reviewed changes

Comment thread packages/nameguard-react/src/utils/text.ts Outdated

Comment thread packages/nameguard-react/src/components/ReportIcon/index.tsx Outdated

Comment thread packages/nameguard-react/src/components/ReportBadge/index.tsx Outdated

vercel Bot deployed to Preview – examples.nameguard.io June 6, 2024 23:14 View deployment

vercel Bot deployed to Preview – nameguard.io June 6, 2024 23:15 View deployment

vercel Bot deployed to Preview – storybook.namekit.io June 6, 2024 23:16 View deployment

FrancoAguzzi and others added 13 commits July 18, 2024 20:16

chore: fix loading data lacking issue

bb13f87

chore: move getNameGuardURLForENSname from NameGuard SDK to NameGuard…

ded3181

… React package

feat: create url file in NameGuard React for utilities

1984de5

refactor: move NameGuard URL related logic to NameGuard React url.ts

6dc768a

chore: commented url file tests

d229db6

feat: changed module resolution of ens-utils

9667053

fix: align build config in examples.nameguard.io

13b08b1

fix: update type file reference of nameguard-react

1e785c0

debug: update ci_sdk.yml to log node_modules content

0b8f598

debug: update back ci_sdk.yml after bug test

06f1706

debug: finish undoing changes to ci_sdk.yml

b643ecf

fix: add vitest to nameguard-react

9afae03

fix: update pnpm-lock file

63d4e55

lightwalker-eth reviewed Jul 19, 2024

View reviewed changes

Comment thread apps/examples.nameguard.io/package.json Outdated

FrancoAguzzi and others added 5 commits July 19, 2024 11:09

fix: update examples.nameguard.io ens-utils version

ef0f788

Merge branch 'main' into feat/create-loading-report-icon

a038605

Merge remote-tracking branch 'origin/main' into feat/create-loading-r…

0cc2a45

…eport-icon

chore: update packages dependencies

f6a54b8

Merge remote-tracking branch 'origin/main' into feat/create-loading-r…

4f82840

…eport-icon

notrab and others added 5 commits August 28, 2024 01:33

Merge branch 'main' into feat/create-loading-report-icon

f9621b3

Merge remote-tracking branch 'origin/main' into feat/create-loading-r…

5e1d864

…eport-icon

feat: import Tooltip from namekit-react in nameguard-react comps

3b7a92e

Merge remote-tracking branch 'origin/main' into feat/create-loading-r…

cb60d00

…eport-icon

feat: update Tooltip imports to be in alignment with new Tooltip expo…

15c15e8

…rt method

lightwalker-eth suggested changes Sep 11, 2024

View reviewed changes

lightwalker-eth added 4 commits September 12, 2024 13:39

Fix PR 282 (#395)

ea54703

* Fix PR 282 * Update sharing and report URL generation * Many additional enhancements * Misc small fixes * Apply suggested update

Undo any changes to price.ts in this branch

766e4c6

Remove changes to ens-utils from this branch

1ceabfe

Remove changes to nameguard-sdk from this branch

bc4192d

		@@ -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 && (

Conversation

FrancoAguzzi commented Jun 1, 2024 • edited by lightwalker-eth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Jun 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Jun 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

lightwalker-eth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lightwalker-eth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lightwalker-eth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FrancoAguzzi commented Aug 27, 2024

Uh oh!

lightwalker-eth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FrancoAguzzi commented Jun 1, 2024 •

edited by lightwalker-eth

Loading

vercel Bot commented Jun 1, 2024 •

edited

Loading

changeset-bot Bot commented Jun 1, 2024 •

edited

Loading