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

fix: 5014 Disable Mint for {{space}} only input #5019

Merged
merged 3 commits into from
Feb 17, 2023

Conversation

maderi21
Copy link
Contributor

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

upper input: space
lower input: empty
image

@maderi21 maderi21 requested a review from a team as a code owner February 15, 2023 15:52
@maderi21 maderi21 requested review from preschian and roiLeo and removed request for a team February 15, 2023 15:52
@kodabot
Copy link
Collaborator

kodabot commented Feb 15, 2023

SUCCESS @maderi21 PR for issue #5014 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Feb 15, 2023

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit c539ceb
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/63ef221d69c6810008814ca2
😎 Deploy Preview https://deploy-preview-5019--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@maderi21
Copy link
Contributor Author

@roiLeo is this solution okay? looks like it's doing what we need

@roiLeo
Copy link
Contributor

roiLeo commented Feb 15, 2023

@roiLeo is this solution okay? looks like it's doing what we need

not the solution I was looking for :/
I don't really like using regex and if I'm correct this one means input shouldn't be empty where there is some BasicInput that can be empty (like in IdentityForm)

@maderi21
Copy link
Contributor Author

maderi21 commented Feb 15, 2023

if I'm correct this one means input shouldn't be empty where there is some BasicInput that can be empty (like IdentityForm)

The regex only checks if the input isn't just space/s. It can stay empty, the prop required takes care of that. If there should be allowed spaces somewhere, such as IdentityForm, we can make this an optional prop.

I don't really like using regex

is there something wrong with regex?

//edit: if the regex isn't okay, can you please point me in a direction of solution that would be acceptable?

@roiLeo
Copy link
Contributor

roiLeo commented Feb 15, 2023

The regex only checks if the input isn't just space/s. It can stay empty, the prop required takes care of that. If there should be allowed spaces somewhere, such as IdentityForm, we can make this an optional prop.

So to understand, with this change I won't be able to create nft with space chars in name?
I thought issue was about empty input but it look like it's working as excepted

I don't really like using regex

is there something wrong with regex?

personal experience it's a pain to maintain and can have side effects

//edit: if the regex isn't okay, can you please point me in a direction of solution that would be acceptable?

maybe @preschian can enlighten us on the original error (are we sure it's not a metadata issue?)

@maderi21
Copy link
Contributor Author

So to understand, with this change I won't be able to create nft with space chars in name?

that wasn't intended, but that's the effect now. reverting back to draft for now.

@maderi21 maderi21 marked this pull request as draft February 15, 2023 16:35
@preschian
Copy link
Member

maybe @preschian can enlighten us on the original error (are we sure it's not a metadata issue?)

@roiLeo yes, I can reproduce it on snek. check this nft metadata https://beta.kodadot.xyz/snek/gallery/185770742-3 and change the ipfs gateway with the other public ipfs gateway

@maderi21:

@maderi21 maderi21 marked this pull request as ready for review February 17, 2023 06:43
@codeclimate
Copy link

codeclimate bot commented Feb 17, 2023

Code Climate has analyzed commit c539ceb and detected 0 issues on this pull request.

View more on Code Climate.

@maderi21
Copy link
Contributor Author

now the regex which doesn't allow spaces is applied only if the field is required and if the contents of the field are only spaces. otherwise, everything is allowed.

@yangwao
Copy link
Member

yangwao commented Feb 17, 2023

pay 30 usd

@yangwao yangwao merged commit 1a4da8e into kodadot:main Feb 17, 2023
@yangwao
Copy link
Member

yangwao commented Feb 17, 2023

😍 Perfect, I’ve sent the payout
💵 $30 @ 39.47 USD/KSM ~ 0.76 $KSM
🧗 JHJGu2aWyAFBfcso8bR7ma82p15YAavyN6vQfJyWuhniNGa
🔗 0x8c0668e5e41c0fd9858a444426d91b4ab5f1ca5a1d831961fc75104a19b95c4f

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable mint when NFT name is empty.
5 participants