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

#4141 On focus in search box show history #4181

Merged
merged 4 commits into from
Oct 28, 2022
Merged

#4141 On focus in search box show history #4181

merged 4 commits into from
Oct 28, 2022

Conversation

prachi00
Copy link
Member

@prachi00 prachi00 commented Oct 26, 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.

Screen Shot 2022-10-25 at 8 47 13 PM

@prachi00 prachi00 requested a review from a team as a code owner October 26, 2022 03:47
@prachi00 prachi00 requested review from Jarsen136 and removed request for a team October 26, 2022 03:47
@kodabot
Copy link
Collaborator

kodabot commented Oct 26, 2022

SUCCESS @prachi00 PR for issue #4141 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 Oct 26, 2022 •
edited
Loading

✅ Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit e6a0eb8
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/635bc1650d32b50009833a0b
😎 Deploy Preview https://deploy-preview-4181--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.

@yangwao
Copy link
Member

yangwao commented Oct 26, 2022

Why I see it on bottom 👀

Screenshot_2022-10-26-14-06-19-89_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

@Jarsen136
Copy link
Contributor

Why I see it on bottom 👀

So do I.
And the count of history result items is always one?

@exezbcz
Copy link
Member

exezbcz commented Oct 26, 2022 •
edited
Loading

Why I see it on bottom 👀

So do I. And the count of history result items is always one?

No, it shouldn't be like that; I was probably thinking about three historical results.
Feedback @prachi00

card

  • text should be 16px
  • cross icon is by default k-grey, on hover it changes color to black
  • the whole card should have a hover effect similar to results cards, (hover bg in dark mode has same color as results card in dark mode) like this(figma):
    image

others

  • History should not be displayed while you write something, thus it is only displayed with the trending collections popup. IMG:
    image

Otherwise, good job!

@prachi00
Copy link
Member Author

  • the whole card should have a hover effect similar to results cards, (hover bg in dark mode has same color as results card in dark mode) like this(figma):

I do see 3 though. do you always see one?
Screen Shot 2022-10-26 at 6 48 19 PM

@prachi00
Copy link
Member Author

Why I see it on bottom 👀

So do I. And the count of history result items is always one?

No, it shouldn't be like that; I was probably thinking about three historical results. Feedback @prachi00

card

  • text should be 16px
  • cross icon is by default k-grey, on hover it changes color to black
  • the whole card should have a hover effect similar to results cards, (hover bg in dark mode has same color as results card in dark mode) like this(figma):
    image

others

  • History should not be displayed while you write something, thus it is only displayed with the trending collections popup. IMG:
    image

Otherwise, good job!

added these

@Jarsen136
Copy link
Contributor

I do see 3 though. do you always see one? Screen Shot 2022-10-26 at 6 48 19 PM

I see 3 items on RMRK now.
However, seems there is no history on BSX and also other chains
image

@exezbcz
Copy link
Member

exezbcz commented Oct 27, 2022

image
At the bottom, there is no rating button, if you focus on search bar in navbar.

@prachi00
Copy link
Member Author

image At the bottom, there is no rating button, if you focus on search bar in navbar.

I dont understand, what rating button?

@yangwao
Copy link
Member

yangwao commented Oct 28, 2022

there is typo in class 👀

@codeclimate
Copy link

codeclimate bot commented Oct 28, 2022

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

View more on Code Climate.

yangwao
yangwao reviewed Oct 28, 2022
View reviewed changes
styles/components/_search.scss
}
}

.search-hisotry-item {
Copy link
Member

@yangwao yangwao Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@prachi00 you can fix typo in supplemental PR please 🙏

Copy link
Member

Choose a reason for hiding this comment

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

could be fixed in another PR I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this yes

yangwao
yangwao reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@yangwao yangwao left a comment

Choose a reason for hiding this comment

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

just class typo )

styles/components/_search.scss
}
}

.search-hisotry-item {
Copy link
Member

Choose a reason for hiding this comment

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

could be fixed in another PR I guess

@yangwao
Copy link
Member

yangwao commented Oct 28, 2022

pay 80 usd

@yangwao yangwao merged commit 99d7c44 into main Oct 28, 2022
@yangwao yangwao deleted the feat-show-history branch October 28, 2022 11:59
@yangwao
Copy link
Member

yangwao commented Oct 28, 2022

😍 Perfect, I’ve sent the payout
💵 $80 @ 34.87 USD/KSM ~ 2.294 $KSM
🧗 EzGc4s9PgCPx1YnF3fqzhLzVHpHMTL4LWPScwpDrR8JKgSU
🔗 0x8117c34389c18f7d5191386f0d8bc7e2b9524947308e1e960308639285a24730

🪅 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 Oct 28, 2022
prachi00 added a commit that referenced this pull request Oct 29, 2022
@prachi00
#4181 fix typo
ce539fc
@prachi00 prachi00 mentioned this pull request Oct 29, 2022
Merged
18 tasks
roiLeo added a commit that referenced this pull request Oct 31, 2022
@roiLeo
Merge pull request #4206 from kodadot/feat-typo
d481e9b 
#4181 fix typo
@yangwao yangwao mentioned this pull request Nov 1, 2022
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangwao yangwao yangwao left review comments

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

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.

On focus in search box we should suggest trending collections
5 participants
@prachi00 @kodabot @yangwao @Jarsen136 @exezbcz