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

refactor: search bar component #3894

Merged
merged 20 commits into from
Sep 13, 2022
Merged

refactor: search bar component #3894

merged 20 commits into from
Sep 13, 2022

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Sep 1, 2022 •
edited
Loading

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

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 </rmrk/collection/26902bc2f7c20c546a-1FVG7>
  • I've tested PR on mobile and everything seems works
  • I found edge cases
  • I've written some unit tests 🧪

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.

@netlify
Copy link

netlify bot commented Sep 1, 2022 •
edited
Loading

✅ Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit cdf9d79
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/631f2ded48655a00089fea14
😎 Deploy Preview https://deploy-preview-3894--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.

@Jarsen136 Jarsen136 marked this pull request as ready for review September 1, 2022 14:12
@Jarsen136 Jarsen136 requested review from a team as code owners September 1, 2022 14:12
@Jarsen136 Jarsen136 requested review from prachi00 and damskyftw and removed request for a team September 1, 2022 14:12
@Jarsen136 Jarsen136 changed the title WIP: refactor: search bar component refactor: search bar component Sep 1, 2022
@roiLeo
Copy link
Contributor

roiLeo commented Sep 1, 2022

Can we have a component with only SearchBar input? without switch, filters & range

@Jarsen136
Copy link
Contributor Author

Can we have a component with only SearchBar input? without switch, filters & range

Yes, that's a good idea.
Already updated.

@petersopko
Copy link
Contributor

@Jarsen136 it's not a range slider anymore, some of the functions/variables may have been left there, but maybe let's call it something more appropriate?

@Jarsen136
Copy link
Contributor Author

@Jarsen136 it's not a range slider anymore, some of the functions/variables may have been left there, but maybe let's call it something more appropriate?

Yup, will check it

@petersopko
Copy link
Contributor

I'd change the names of two files:
SearchBar.vue -> Search.vue
SearchBarInput.vue -> SearchBar.vue
ref:
image

@Jarsen136
Copy link
Contributor Author

I'd change the names of two files: SearchBar.vue -> Search.vue SearchBarInput.vue -> SearchBar.vue ref: ![image]

Done!

@prachi00 prachi00 added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Sep 4, 2022
@petersopko
Copy link
Contributor

@Jarsen136 the last thing here is, let's try to fix the tests for Buy Now button, I assume something was changed in this regard and that's why the tests are failing right now.

@petersopko
Copy link
Contributor

@vikiival this lgtm, can you take a look ? @prachi00 said it's good, but I'm not sure about the length of the file (or whether there's a possibility for shortening it further)

vikiival
vikiival reviewed Sep 12, 2022
View reviewed changes
components/search/Search.vue
@@ -0,0 +1,365 @@
<template>
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have like 400 LOC?

vikiival
vikiival requested changes Sep 12, 2022
View reviewed changes
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.

Please do more proper code split in components/search/Search.vue

It still contains like 400 LOC

components/search/SearchCollection.vue Outdated Show resolved Hide resolved
@vikiival
Copy link
Member

but I'm not sure about the length of the file

Yeah it's too long

@petersopko petersopko removed the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Sep 12, 2022
@Jarsen136
Copy link
Contributor Author

Please do more proper code split in components/search/Search.vue

It still contains like 400 LOC

I have tried to reduce it to about 300 LOC : )

@Jarsen136 Jarsen136 requested a review from vikiival September 12, 2022 11:23
@Jarsen136 Jarsen136 mentioned this pull request Sep 12, 2022
Closed
@codeclimate
Copy link

codeclimate bot commented Sep 12, 2022

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

View more on Code Climate.

@yangwao
Copy link
Member

yangwao commented Sep 12, 2022

Why it's showing the third collection when there is no "Moon" in the name of the collection?

image

@Jarsen136
Copy link
Contributor Author

Why it's showing the third collection when there is no "Moon" in the name of the collection?

image

For the third collection https://deploy-preview-3894--koda-nuxt.netlify.app/movr/collection/0x2f26efdb7233a014715ce6e895aa67d846d93f1e.

image

Its original name is MoonShroomiz so that it's showing in the search result.
After fetching the metadata of the this collection https://kodadot.mypinata.cloud/ipfs/QmXvfjHAiaJHTu5692YofHZunvRJdZiPpcepw8pu4B3wuf/1.json.

You could see that its final name is Patient Naoko.
image

IMO it's a bug related to the subsquid 002-rc0 side. @vikiival Could you pls take a look 👀

@vikiival
Copy link
Member

IMO it's a bug related to the subsquid 002-rc0 side.

kodadot/click#34

@yangwao yangwao mentioned this pull request Sep 13, 2022
Open
@yangwao
Copy link
Member

yangwao commented Sep 13, 2022

Merging in prior to continue to bring search bar on the landing page - #3844

@yangwao
Copy link
Member

yangwao commented Sep 13, 2022

pay 100

@yangwao yangwao merged commit 744cda5 into kodadot:main Sep 13, 2022
@yangwao
Copy link
Member

yangwao commented Sep 13, 2022

😍 Perfect, I’ve sent the payout
💵 $100 @ 51.04 USD/KSM ~ 1.959 $KSM
🧗 Caiv9TbPz68q5dC8EcHu5xKYPRnremimGzqmEejDFNpWWLG
🔗 0xd3c42832c1ca83014101948aeb017d49d60853ec60b8d6d01c9559b38c5d04e4

🪅 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 Sep 13, 2022
@petersopko petersopko mentioned this pull request Sep 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prachi00 prachi00 prachi00 approved these changes

@damskyftw damskyftw Awaiting requested review from damskyftw damskyftw was automatically assigned from kodadot/qa-guild

@vikiival vikiival Awaiting requested review from vikiival

Assignees
No one assigned
Labels
paid pull-request has been paid
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor: SearchBar - splitting into smaller components
6 participants
@Jarsen136 @roiLeo @petersopko @vikiival @yangwao @prachi00