Skip to content
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

NFT mints #146

Merged
merged 6 commits into from
Jan 12, 2024
Merged

NFT mints #146

merged 6 commits into from
Jan 12, 2024

Conversation

whilefoo
Copy link
Contributor

still in the works

@whilefoo whilefoo marked this pull request as ready for review December 31, 2023 13:56
Copy link

ubiquibot bot commented Dec 31, 2023

@rndquu
Copy link
Member

rndquu commented Jan 3, 2024

@whilefoo So with this PR https://pay.ubq.fi accepts (in a query param) an array of exactly 2 values: our "traditional" permit and NFT mint request, right?

@whilefoo
Copy link
Contributor Author

whilefoo commented Jan 4, 2024

@whilefoo So with this PR https://pay.ubq.fi accepts (in a query param) an array of exactly 2 values: our "traditional" permit and NFT mint request, right?

it accepts any number of permits and NFT mint requests

@0x4007
Copy link
Member

0x4007 commented Jan 6, 2024

Looks like this is good to go? Do you think it's finished @whilefoo ?

@whilefoo
Copy link
Contributor Author

whilefoo commented Jan 7, 2024

Looks like this is good to go? Do you think it's finished @whilefoo ?

Yes, it's good to go

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I would handle this cosmetic change manually but I'm traveling at the moment and would rather get this merged in asap.

I noticed that the text for the new DOM elements is white, and when using the UI with light mode enabled, the text is very difficult to see. Please be sure to test by changing your OS settings to light mode and handling the CSS correctly.

The most "correct" way to handle this might be to refer to my other repository where I automatically generate an inverted brightness stylesheet during esbuild. I implemented this in work.ubq.fi repository.

If that's too complicated, then please just manually address your new DOM element colors.

@@ -0,0 +1,5 @@
export function removeAllEventListeners(element: Element): Element {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must remove the event listener manually. Deleting an element with an event listener from the DOM is not sufficient. I verified this with a quick perplexity search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the mintNftHandler returns a new function every time but if you want to remove an event listener you need to give it the function that mintNftHandler returned when it was registered. It's not enough to call mintNftHandler again with the same parameters because it returns a new function so it doesn't work.

I'm pretty sure cloning an element and replacing it with the old one removes all event listeners because I tested it. You can also see here.

Copy link
Member

@0x4007 0x4007 Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing on that page that says the event listener is removed when an element is removed from the DOM.

The problem is the mintNftHandler returns a new function every time

I don't follow. Are you saying it's hard to maintain a reference to the correct function to remove it as an event listener handler? I'm sure you can make an object at the highest scope and just use that as a cache. It doesn't seem like a complex problem.

@whilefoo
Copy link
Contributor Author

The most "correct" way to handle this might be to refer to my other repository where I automatically generate an inverted brightness stylesheet during esbuild. I implemented this in work.ubq.fi repository.

I tried this but I had some issues with it because in work.ubq.fi there's only one CSS file but here we have multiple files that get imported by the main file for example rewards.css:

@import url("pay.css");
@import url("background.css");
@import url("claim-table.css");
@import url("media-queries.css");
@import url("../proxima.css");
@import url("../toast.css");
@import url("../fa.css");
@import url("light-mode.css");

and because esbuild runs the plugin only on the main CSS files it just creates rewards-inverted.css which has the same content as rewards.css and esbuild merges all these files into one CSS file but the plugin only processes input files and not output files.
Also we never actually use the esbuild output - when we deploy to Cloudflare we use the folder static, but the build folder is static/out.

I temporarily fixed the light mode manually and I will have to create a custom script that creates inverted CSS after the esbuild finishes its build, or if you have any other ideas how we can do that with the esbuild plugin.

@0x4007
Copy link
Member

0x4007 commented Jan 12, 2024

image

The latest link doesn't seem to be fixed

https://814e7c5e.pay-ubq-fi-bxp.pages.dev

@0x4007
Copy link
Member

0x4007 commented Jan 12, 2024

Oh weird the ordering is backwards. We should fix this to display in chronological order.

image

@0x4007 0x4007 merged commit 057a657 into ubiquity:development Jan 12, 2024
1 check passed
@0x4007
Copy link
Member

0x4007 commented Jan 22, 2024

Doesn't it make sense to hide the NFT rewards UI element unless we are passing in NFT reward related information in the query string? @whilefoo

Comment on lines +41 to +60
const NftMint = T.Object({
type: T.Literal("nft-mint"),
request: T.Object({
beneficiary: TAddress,
deadline: TBigNumber,
keys: T.Array(T.String()),
nonce: TBigNumber,
values: T.Array(T.String()),
}),
nftMetadata: T.Object({
GITHUB_ORGANIZATION_NAME: T.String(),
GITHUB_REPOSITORY_NAME: T.String(),
GITHUB_ISSUE_ID: T.String(),
GITHUB_USERNAME: T.String(),
GITHUB_CONTRIBUTION_TYPE: T.String(),
}),
nftAddress: TAddress,
networkId: TNetworkId,
signature: TSignature,
});
Copy link
Member

@0x4007 0x4007 Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having type issues due to trying to process ERC20 and ERC721 permits in the UI code (the schemas don't line up at all.)

We should try and sync the properties as much as possible. I'm working on that now on my branch. I hope that it won't cause issues but I'm looking for input on constraints for the property names.

From what I understand if we change the property names then the old permits will be incompatible.

@whilefoo why not just copy the schema as much as possible of ERC20 permits to the ERC721 permits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because then you have erc721 specific properties like nftAddress, nftMetadata in erc20 type and the other way around so it's confusing

Copy link
Member

@0x4007 0x4007 Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add an extra property specifically for NFTs then. That way its very easy to deal with the generic Permit type, and then extend for NftPermit

I only need to make the UI code compatible with the base Permit type and we are good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants