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

#5852 Neo Input and Neo Field #5870

Merged
merged 36 commits into from
May 31, 2023
Merged

#5852 Neo Input and Neo Field #5870

merged 36 commits into from
May 31, 2023

Conversation

prachi00
Copy link
Member

@prachi00 prachi00 commented May 5, 2023

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

  • Bugfix
  • Feature
  • Refactoring

Context

  • Closes #
  • Requires deployment <snek/rubick/worker>

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

Optional

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

Had issue bounty label?

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.
    SS
    Screen Shot 2023-05-08 at 12 33 33 PM
    Screen Shot 2023-05-08 at 12 33 50 PM

Screen Shot 2023-05-08 at 12 34 09 PM
Screen Shot 2023-05-08 at 12 34 37 PM

Copilot Summary

🤖 Generated by Copilot at e1b9ba3

No summary available (Limit exceeded: required to process 75846 tokens, but only 50000 are allowed per call)

🤖 Generated by Copilot at 7b7ed90

We are the NeoFields, we rise from the ashes
We replace the b-fields, we bring the new flashes
We are the NeoFields, we rule the forms and grids
We crush the b-fields, we are the Neo kids

@prachi00 prachi00 requested a review from a team as a code owner May 5, 2023 20:02
@prachi00 prachi00 requested review from roiLeo and vikiival and removed request for a team May 5, 2023 20:02
@netlify
Copy link

netlify bot commented May 5, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 331c471
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64765187e08abe00080a625e
😎 Deploy Preview https://deploy-preview-5870--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 settings.

@prachi00 prachi00 mentioned this pull request May 5, 2023
37 tasks
@reviewpad reviewpad bot added large Pull request is large waiting-for-review labels May 5, 2023
@reviewpad
Copy link
Contributor

reviewpad bot commented May 5, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

@vikiival
Copy link
Member

vikiival commented May 6, 2023

@prachi00 can you provide screenshot pls?

@prachi00
Copy link
Member Author

prachi00 commented May 8, 2023

SS

@prachi00 can you provide screenshot pls?

added some

@vikiival
Copy link
Member

@prachi00 can you please resolve conflicts?

@prachi00
Copy link
Member Author

@prachi00 can you please resolve conflicts?

done

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

~

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

please could you check

  • type="is-... => variant="..."
  • remove message prop on NeoField

components/explore/ExploreGrid.vue Outdated Show resolved Hide resolved
components/rmrk/Create/SimpleMint.vue Show resolved Hide resolved
@vikiival
Copy link
Member

Agree with @roiLeo

prachi00 and others added 2 commits May 16, 2023 09:26
Co-authored-by: roiLeo <medina.leo42@gmail.com>
Co-authored-by: roiLeo <medina.leo42@gmail.com>
@vikiival
Copy link
Member

@kodadot/qa-guild is it possible to check this ? Seems quite a big change 😶

@prachi00
Copy link
Member Author

it seems theres an icon that isnt showing when form is invalid and its taking up all the space

there is no icon named alert-circle on fa https://fontawesome.com/icons/alert-circle?s=solid. could you change to something else?

I didnt add it though, this icon was added by oruga, I'll look into replacing it if we can

@prachi00
Copy link
Member Author

fixed the conflicts again, can we merge now, I'll do the rest of styling changes in followup issue
@vikiival

@vikiival
Copy link
Member

@prachi00 can you please open the follow-up issue with changes that needs to be done?

@prachi00
Copy link
Member Author

@prachi00 can you please open the follow-up issue with changes that needs to be done?

done

Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

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

@prachi00 to mock matchmedia in the failed test

Object.defineProperty(window, 'matchMedia', {
  writable: true,
  value: vi.fn().mockImplementation((query) => ({
    matches: false,
    media: query,
    onchange: null,
    addListener: vi.fn(),
    removeListener: vi.fn(),
    addEventListener: vi.fn(),
    removeEventListener: vi.fn(),
    dispatchEvent: vi.fn(),
  })),
})

I didnt add it though, this icon was added by oruga, I'll look into replacing it if we can

yes, that one default value from oruga. need to change that in the seperate PR https://github.com/oruga-ui/oruga/blob/develop/packages/oruga/src/utils/FormElementMixin.js#L113

otherwise lgtm

@vikiival vikiival enabled auto-merge (squash) May 29, 2023 16:23
@vikiival vikiival disabled auto-merge May 29, 2023 16:23
@vikiival vikiival enabled auto-merge (squash) May 29, 2023 16:24
@vikiival
Copy link
Member

So 🥺

@roiLeo
Copy link
Contributor

roiLeo commented May 30, 2023

So 🥺

Screenshot 2023-05-30 at 15-13-21 #5852 Neo Input and Neo Field by prachi00 · Pull Request #5870 · kodadot_nft-gallery

knock-doorknob

@prury prury added S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked and removed S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels May 30, 2023
@prachi00
Copy link
Member Author

resolved conflicts again, can be merged now

@codeclimate
Copy link

codeclimate bot commented May 30, 2023

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

View more on Code Climate.

@prachi00
Copy link
Member Author

prachi00 commented May 30, 2023

Also, e2e test cases are failing on main now, I have fixed some of them in this pr by removing it as I think it doesnt exist anymore, I hope that is correct, more are failing but I want to confirm that removing them is the right way to fix it?
let me know if I need to revert those changes if we need them, i'll do it 🫣
CC @preschian @vikiival @roiLeo

@prachi00
Copy link
Member Author

Also, e2e test cases are failing on main now, I have fixed some of them in this pr by removing it as I think it doesnt exist anymore, I hope that is correct, more are failing but I want to confirm that removing them is the right way to fix it? let me know if I need to revert those changes if we need them, i'll do it 🫣 CC @preschian @vikiival @roiLeo

PS can we merge this pr now please, as the test cases failing are not related to this anyway

@vikiival vikiival merged commit c8e3e60 into main May 31, 2023
@vikiival vikiival deleted the feat-neo-input branch May 31, 2023 07:47
@vikiival
Copy link
Member

I hope that is correct,

So that means you don't propagate them?

@prachi00
Copy link
Member Author

I hope that is correct,

So that means you don't propagate them?

what do you mean propagate?

@vikiival
Copy link
Member

what do you mean propagate?

What was the reason to remove lines from the tests.
And how to bring the back

@prachi00
Copy link
Member Author

what do you mean propagate?

What was the reason to remove lines from the tests. And how to bring the back

because they did not exist and thats why they failed, do we even want them back?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants