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.

Sign up for GitHub

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

Jump to bottom

Not connected wallet collection selection #7386

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Not connected wallet collection selection #7386

merged 6 commits into from
Sep 28, 2023

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Sep 27, 2023 •
edited
Loading

Thank you for your contribution to the KodaDot - One Stop Shop for Polkadot NFTs.

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

PR Type

  • Feature
  • Refactor: unify the collection selection process (mass mint & single nft mint )

Needs Design check

Context

Did your issue had any of the "$" label on it?

Screenshot 📸

  • My fix has changed UI

Copilot Summary

🤖 Generated by Copilot at 866ce39

Improved the user experience of the create landing page by requiring login before redirecting to different creation paths. Used a custom function and a hook to handle the login and redirection logic in CreateLanding.vue.

🤖 Generated by Copilot at 866ce39

No more links to guide your way
Only divs that make you pay
If you want to create your fate
You must login or face the gate

@Jarsen136 Jarsen136 requested a review from a team as a code owner September 27, 2023 15:32
@Jarsen136 Jarsen136 requested review from preschian and floyd-li and removed request for a team September 27, 2023 15:32
@kodabot
Copy link
Collaborator

kodabot commented Sep 27, 2023

SUCCESS @Jarsen136 PR for issue #7382 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 Sep 27, 2023 •
edited
Loading

✅ Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 273cd89
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/6514ed9ecc77210008b3725e
😎 Deploy Preview https://deploy-preview-7386--koda-canary.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 configuration.

@reviewpad
Copy link
Contributor

reviewpad bot commented Sep 27, 2023

AI-Generated Summary: This pull request, authored by Jarsen, includes several modifications to the 'CreateLanding.vue' file in order to fix an issue related to the selection of a collection when the wallet is not connected. Modifications primarily consist of changing instances of 'nuxt-link' to 'div', and then applying a specific 'onclick' action which directs the user to the appropriate route after login. In doing so, the patch ensures that the user is successfully logged in before routing them, thus mitigating the error previously encountered.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Sep 27, 2023
@daiagi
Copy link
Contributor

daiagi commented Sep 27, 2023

Can we add "create collection" in the "select collection" drop down in the nft creation form as well?

Like in massmint

@exezbcz
Copy link
Member

exezbcz commented Sep 27, 2023

Can we add "create collection" in the "select collection" drop down in the nft creation form as well?

+1

plus we have a hole in this. When a user connects its wallet and then proceed to the page, he can still disconnect

  • for this, could you please make the button label "please connect your wallet"
    image

  • and one small detail, 16px text please

@Jarsen136 Jarsen136 marked this pull request as draft September 27, 2023 16:34
@reviewpad reviewpad bot added medium Pull request is medium and removed small Pull request is small labels Sep 27, 2023
@Jarsen136 Jarsen136 marked this pull request as ready for review September 27, 2023 17:24
@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Sep 27, 2023 •
edited
Loading

Can we add "create collection" in the "select collection" drop down in the nft creation form as well?

+1

plus we have a hole in this. When a user connects its wallet and then proceed to the page, he can still disconnect

  • for this, could you please make the button label "please connect your wallet"
    image
  • and one small detail, 16px text please

✅ Done

I have also unified the "select collection" process in both massmint and nft creation so that we could delete some duplicate code.

mass mint:
image

nft creation:
image

daiagi
daiagi approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@daiagi daiagi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested on mobile with nova wallet
Wfm

Maybe move chooseCollectionDropdown outside of massmint?

Other than that lgtm

@Jarsen136
Copy link
Contributor Author

Tested on mobile with nova wallet Wfm

Maybe move chooseCollectionDropdown outside of massmint?

Other than that lgtm

Sure. I have moved it. ✅

@prury
Copy link
Member

prury commented Sep 27, 2023

Showing Create Nft instead of Creating Collecion:
image

@Jarsen136
Copy link
Contributor Author

Showing Create Nft instead of Creating Collecion

fixed

@exezbcz
Copy link
Member

exezbcz commented Sep 27, 2023

one last detail, could you please remove the drop shadow from the selector - keeping the rule of no double drop shadow
image

@prury
Copy link
Member

prury commented Sep 27, 2023

why do you fail my dear playwright, even if the Not Enough Funds message is still there

@codeclimate
Copy link

codeclimate bot commented Sep 28, 2023

Code Climate has analyzed commit 273cd89 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@sonarcloud
Copy link

sonarcloud bot commented Sep 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
1.0% 1.0% Duplication

@Jarsen136
Copy link
Contributor Author

why do you fail my dear playwright, even if the Not Enough Funds message is still there

I fixed it. It's because the message changes to "please connect your wallet" when a user does not log in. I have added a login process to this e2e test case, and then it works.

@Jarsen136
Copy link
Contributor Author

one last detail, could you please remove the drop shadow from the selector - keeping the rule of no double drop shadow image

✅ Done

@prury
Copy link
Member

prury commented Sep 28, 2023

why do you fail my dear playwright, even if the Not Enough Funds message is still there

I fixed it. It's because the message changes to "please connect your wallet" when a user does not log in. I have added a login process to this e2e test case, and then it works.

oh my friend, thank you so much!

@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Sep 28, 2023
@yangwao yangwao merged commit ffc767b into kodadot:main Sep 28, 2023
13 of 14 checks passed
This was referenced Sep 30, 2023
Merged
Merged
Merged
Merged
This was referenced Oct 11, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roiLeo roiLeo roiLeo left review comments

@daiagi daiagi daiagi approved these changes

@preschian preschian Awaiting requested review from preschian preschian is a code owner automatically assigned from kodadot/code-review-guild

@floyd-li floyd-li Awaiting requested review from floyd-li floyd-li is a code owner automatically assigned from kodadot/code-review-guild

Assignees
No one assigned
Labels
medium Pull request is medium S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked waiting-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Not connected wallet collection selection
7 participants
@Jarsen136 @kodabot @daiagi @exezbcz @prury @roiLeo @yangwao