Research/improve dx integration #28

pgarciaegido · 2022-02-19T16:45:10Z

Here I make significant changes to the way our SDK is integrated. Here are the main improvements, and the nitty gritty on how they work.

Provider is no longer needed on the root of the customers application

How?

I have moved the state from the providers all the way down to the button. State is handled in two different hooks:

packages/ui/react-ui/src/hooks/useCrossMintStatus.ts: Here we handle the status of the client, from the client-id provided.
packages/ui/react-ui/src/hooks/useCrossMintModal.ts : Here we handle the state of the popup window.

We used to have a React component that created an overlay on the page, and its state was handled by a context at the provider level. Now, we manually create and append the div to the body when the user clicks on the crossmint button, and remove it when popup is closed. And actually, this is now optional (true by default). See more at the bottom.

Styles are no longer required to be attached manually

How?

Using a CSS in JS library (styled-components) that would bind the styles to the bundled output files. This actually simplifies a lot the customization of the button in the user side. We can leverage css specification and simply use button.<custom-class> in order to remove the need to use !importants. (See packages/starter/nextjs-starter/styles/globals.css)

New options to the button

Now the button is taking also the clientId prop, but also the development and some others (check packages/ui/react-ui/package/src/CrossMintButton.tsx interface).

I have added 3 more (optionals):

showOverlay True by default. Will show an overlay when the popup window is open and will remove it when its closed (like it works now). If false, button text will continue to change but overlay wont be shown.
crossmintOpened This expects a callback function that will be called everytime the popup window opens. This is handy for the devs to want to show a custom level or just to be able to track the state of the window.
crossmintClosed This expects a callback function that will be called everytime the popup window closes.

Downsides of this approach

I would say a downside is using and external library for the css in js. Now our g-ziped bundle size is around ~13kb which is not crazy, but certainly could be much lower. I might need to do a little research on finding another ligther css in js lib.
Not having a provider (crossmint global state available) and having to use callbacks to control the state of the button might be annoying to some more experienced devs that want to be able to check on the crossmint button anywhere on their app. There is a trade off here.

BTW guys, you see that I have created a pack script and linked the package manually on the next.js starter... I dont know if there is any other way you guys do that (maybe we could use Storybook for this?), but that will be removed if this goes forward.

⚠️ This needs to be tested more in detail before merging!

Thanks a lot!

pgarciaegido · 2022-02-19T16:46:22Z

packages/ui/react-ui/package.json

    ],
    "publishConfig": {
        "access": "public"
    },
    "scripts": {
        "clean": "shx rm -rf lib/*",
        "build": "yarn clean && tsc -p tsconfig.json && tsc -p tsconfig-cjs.json",
-        "postbuild": "shx echo '{ \"type\": \"commonjs\" }' > lib/cjs/package.json && shx echo '{ \"type\": \"module\" }' > lib/esm/package.json"
+        "postbuild": "shx echo '{ \"type\": \"commonjs\" }' > lib/cjs/package.json && shx echo '{ \"type\": \"module\" }' > lib/esm/package.json",
+        "pack": "yarn build && yarn pack --filename package.tgz && tar -xf package.tgz"


Here Im bulding the package locally in order to test in the nextjs starter.

Lets not do this lol. You just need to run yarn i believe, and the packages are linked automatically

yep yep, will delete 😄

mPaella · 2022-02-19T17:33:35Z

packages/starter/nextjs-starter/package.json

    },
    "dependencies": {
-        "@crossmint/client-sdk-react-ui": "^0.0.5",
+        "@crossmint/client-sdk-react-ui": "file:/Users/pgarciaegido/Desktop/code/paella/crossmint-client-sdk/packages/ui/react-ui/package",


this is not necessary, its done by lerna automatically using npm-link

mPaella · 2022-02-19T17:34:07Z

packages/starter/nextjs-starter/pages/index.tsx

@@ -15,6 +15,8 @@ export default function Index() {
                collectionTitle="<TITLE_FOR_YOUR_COLLECTION>"
                collectionDescription="<DESCRIPTION_OF_YOUR_COLLECTION>"
                collectionPhoto="<OPT_URL_TO_PHOTO_COVER>"
+                clientId="12345"


this is causing a shit ton of emails to be sent to us during your testing FYI lol

We should do #14 to fix this for good

And modify this to say "<YOUR_CLIENT_ID>"

mPaella · 2022-02-19T17:35:12Z

packages/starter/nextjs-starter/pages/index.tsx

@@ -15,6 +15,8 @@ export default function Index() {
                collectionTitle="<TITLE_FOR_YOUR_COLLECTION>"
                collectionDescription="<DESCRIPTION_OF_YOUR_COLLECTION>"
                collectionPhoto="<OPT_URL_TO_PHOTO_COVER>"
+                clientId="12345"
+                className="my-crossmint-button"


Most people aren't going to understand that they need to remove this, they already don't even edit the props above...

+1. Maybe we can have a non customized button, and then a separate customized one? And add a comment in the cusotmized one explaining how it works

Also I would rename the classname to "my-custom-style-crossmint-button" or something like that, super obvious

I would not give it up tbh. It makes the most sense to me at least. How about we just remove it from the examples and snippets, and maybe add a little comment to it in the very bottom of the docs?

Most good devs will just look here, or look at the ts type and see they can use this

But seems like a lot of the people we r dealing with are not good devs

Yeah... I mean to keep the prop as className.

Obviously, it needs to be gone from the example haha

Yeah the prop seems fine, my comment (and I think Maxwell's) was about keeping it in the main example

mPaella · 2022-02-19T17:38:14Z

packages/ui/react-ui/package.json

    ],
    "publishConfig": {
        "access": "public"
    },
    "scripts": {
        "clean": "shx rm -rf lib/*",
        "build": "yarn clean && tsc -p tsconfig.json && tsc -p tsconfig-cjs.json",
-        "postbuild": "shx echo '{ \"type\": \"commonjs\" }' > lib/cjs/package.json && shx echo '{ \"type\": \"module\" }' > lib/esm/package.json"
+        "postbuild": "shx echo '{ \"type\": \"commonjs\" }' > lib/cjs/package.json && shx echo '{ \"type\": \"module\" }' > lib/esm/package.json",
+        "pack": "yarn build && yarn pack --filename package.tgz && tar -xf package.tgz"


Lets not do this lol. You just need to run yarn i believe, and the packages are linked automatically

mPaella · 2022-02-19T17:39:50Z

packages/ui/react-ui/src/CrossMintButton.tsx

-    const { hideMintOnInactiveClient, status } = useCrossMintStatus();
-
-    const { connecting, connect } = useCrossMintPopup();
-
    const handleClick: MouseEventHandler<HTMLButtonElement> = useCallback(
        (event) => {
            if (onClick) onClick(event);


If we are using crossmintOpened lets remove onClick

mPaella · 2022-02-19T17:40:49Z

packages/ui/react-ui/src/CrossMintButton.tsx

+    hideMintOnInactiveClient?: boolean;
+    clientId: string;
+    development?: boolean;
+    crossmintOpened?: () => any;


Nit, but i think onCrossmintOpened and onCrossmintClosed is more standard practice for a function handler

If these are new parameters that didn't exist before, can we take them out of this PR and send a different PR for them? OR file an issue to track

I'd want more time to define the API surface properly, but didn't want to block this PR from going out in the meantime

Ok @alfonso-paella lets do that 😄

Created #34

alfonso-paella · 2022-02-19T18:17:47Z

packages/ui/react-ui/src/hooks/useCrossMintModal.ts

+const PROD_URL = "https://www.crossmint.io";
+const DEV_URL = "http://localhost:3001";
+
+const executeIfExists = (fn?: () => any | undefined):void => {


This file doesn't seem to have been run through prettier

We dont have prettier set up in this repo I believe

Oops, we should add it then. Mind running it manually for now?

alfonso-paella · 2022-02-19T18:18:42Z

packages/ui/react-ui/src/hooks/useCrossMintModal.ts

+            executeIfExists(crossmintOpened);
+        } else {
+            setConnecting(false);
+            console.log("Failed to open popup window");


Can we make this a console.error and have it be more actionable for the developer? Can they do anything to fix this

sure! @mPaella anything comes to mind?

If it's not obvious let's leave a TODO and do in another task

pgarciaegido · 2022-02-19T21:48:30Z

onOpen and onClose props already deleted 👍

alfonso-paella · 2022-02-20T03:39:54Z

Mind adding back the onclick ? Until we figure out what to do with the onopen and onclose

alfonso-paella

I'm good pending the couple unresolved comments. Once you address those feel free to merge if you feel comfortable

alfonso-paella · 2022-02-20T03:38:22Z

packages/starter/nextjs-starter/next.config.js

@@ -0,0 +1,5 @@
+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const withBundleAnalyzer = require('@next/bundle-analyzer')({
+  enabled: process.env.ANALYZE === 'true',


Run prettier in this file

pgarciaegido added 9 commits February 19, 2022 12:04

providerless approach

ac63c02

changes

4c6f04a

little refactor

e13c2fd

Adding overlay

0330222

adding spinner

f7baae9

some more changes

2e47251

changes

b3132e4

styling

8cd0a6f

remove package.tgz from git

5e2b306

pgarciaegido commented Feb 19, 2022

View reviewed changes

pgarciaegido added 2 commits February 19, 2022 18:04

add analyzer

27d6a14

lock

dce21d8

pgarciaegido requested review from mPaella and alfonso-paella February 19, 2022 17:29

mPaella requested changes Feb 19, 2022

View reviewed changes

alfonso-paella requested changes Feb 19, 2022

View reviewed changes

pgarciaegido mentioned this pull request Feb 19, 2022

[Feature req] Add APIs for popup opened, closed callback #34

Open

pgarciaegido added 4 commits February 19, 2022 21:29

requested changes

1dee948

format docs

4b4af77

remove onCrossmintOpen and onCrossmintClose

79032be

solve conflicts

8f50857

alfonso-paella mentioned this pull request Feb 19, 2022

[DevExp] More graceful deprecation of old button #36

Open

alfonso-paella approved these changes Feb 20, 2022

View reviewed changes

last requested changes

67ead1f

pgarciaegido merged commit ad4c934 into main Feb 20, 2022

pgarciaegido deleted the research/improve-dx-integration branch February 20, 2022 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Research/improve dx integration #28

Research/improve dx integration #28

pgarciaegido commented Feb 19, 2022 •

edited

Loading

pgarciaegido Feb 19, 2022

mPaella Feb 19, 2022

pgarciaegido Feb 19, 2022

mPaella Feb 19, 2022

mPaella Feb 19, 2022

alfonso-paella Feb 19, 2022

alfonso-paella Feb 19, 2022

pgarciaegido Feb 19, 2022

mPaella Feb 19, 2022

alfonso-paella Feb 19, 2022

alfonso-paella Feb 19, 2022

pgarciaegido Feb 19, 2022

mPaella Feb 19, 2022

pgarciaegido Feb 19, 2022

alfonso-paella Feb 19, 2022

mPaella Feb 19, 2022

mPaella Feb 19, 2022

mPaella Feb 19, 2022

alfonso-paella Feb 19, 2022

pgarciaegido Feb 19, 2022

pgarciaegido Feb 19, 2022

alfonso-paella Feb 19, 2022

pgarciaegido Feb 19, 2022

alfonso-paella Feb 19, 2022

alfonso-paella Feb 19, 2022

pgarciaegido Feb 19, 2022

alfonso-paella Feb 20, 2022

pgarciaegido commented Feb 19, 2022 •

edited

Loading

alfonso-paella commented Feb 20, 2022

alfonso-paella left a comment

alfonso-paella Feb 20, 2022

Research/improve dx integration #28

Research/improve dx integration #28

Conversation

pgarciaegido commented Feb 19, 2022 • edited Loading

Provider is no longer needed on the root of the customers application

How?

Styles are no longer required to be attached manually

How?

New options to the button

Downsides of this approach

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgarciaegido commented Feb 19, 2022 • edited Loading

alfonso-paella commented Feb 20, 2022

alfonso-paella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgarciaegido commented Feb 19, 2022 •

edited

Loading

pgarciaegido commented Feb 19, 2022 •

edited

Loading