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

feat: full screen previewer #5321

Merged
merged 15 commits into from
Mar 24, 2023
Merged

feat: full screen previewer #5321

merged 15 commits into from
Mar 24, 2023

Conversation

floyd-li
Copy link
Member

Thank you for your contribution to the KodaDot NFT gallery.

this pr is to add the full screen previewer feature on gallery item, also fix some display issue i commited in #5309

FYI: @prachi00 since the break point is set to 930px so i remove the styles you commit in #5134, it caused some display issue in new break point

👇 _ 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.

image

image

image

image

@floyd-li floyd-li requested a review from a team as a code owner March 22, 2023 06:33
@floyd-li floyd-li requested review from roiLeo and Jarsen136 and removed request for a team March 22, 2023 06:33
@kodabot
Copy link
Collaborator

kodabot commented Mar 22, 2023

WARNING @floyd-li PR for issue #4764 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #4764

@netlify
Copy link

netlify bot commented Mar 22, 2023 •
edited
Loading

✅ Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 5940a78
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/641d48ce61ca7400086c7e83
😎 Deploy Preview https://deploy-preview-5321--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.

@floyd-li
Copy link
Member Author

@exezbcz please do the ui check :)

roiLeo
roiLeo reviewed Mar 22, 2023
View reviewed changes
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.

Hey, small note

components/gallery/GalleryItem.vue Outdated Show resolved Hide resolved
@exezbcz
Copy link
Member

exezbcz commented Mar 22, 2023 •
edited
Loading

hey hey, nice
image

the left is for darkmode, the left for light mode
image

@roiLeo
Copy link
Contributor

roiLeo commented Mar 22, 2023

Why did you change NeoIcon to use svg? I prefer that you use fontawesome expand icon since it's the same and then change color according to theme

@floyd-li
Copy link
Member Author

Why did you change NeoIcon to use svg? I prefer that you use fontawesome expand icon since it's the same and then change color according to theme

reverted using NeoIcon @roiLeo

@reviewpad reviewpad bot mentioned this pull request Mar 22, 2023
Merged
@exezbcz
Copy link
Member

exezbcz commented Mar 22, 2023

  • can you please make the button visible on all touch devices - not only mobile
    image

roiLeo
roiLeo reviewed Mar 23, 2023
View reviewed changes
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.

otherwise wfm ✅

components/gallery/GalleryItem.vue Outdated Show resolved Hide resolved
components/gallery/GalleryItem.vue Outdated Show resolved Hide resolved
@exezbcz
Copy link
Member

exezbcz commented Mar 23, 2023

last thing:

can you please make the button visible on all touch devices - not only mobile

this still does not work, it should be shown by default, not after click on the img

otherwise good!

@floyd-li
Copy link
Member Author

updated

roiLeo
roiLeo approved these changes Mar 23, 2023
View reviewed changes
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.

✅ wfm

components/gallery/GalleryItem.vue Outdated Show resolved Hide resolved
@roiLeo roiLeo added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Mar 23, 2023
roiLeo
roiLeo suggested changes Mar 23, 2023
View reviewed changes
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.

Mobile navbar changed

beta
Screenshot 2023-03-23 at 17-51-32 Mountain KodaDot - NFT Market Explorer

preview
Screenshot 2023-03-23 at 17-51-47 Mountain KodaDot - NFT Market Explorer

@floyd-li @roiLeo
Update components/gallery/GalleryItem.vue
5a3c664 
Co-authored-by: roiLeo <medina.leo42@gmail.com>
Jarsen136
Jarsen136 reviewed Mar 24, 2023
View reviewed changes
Copy link
Contributor

@Jarsen136 Jarsen136 left a comment

Choose a reason for hiding this comment

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

some small stuffs. Otherwise lgtm

components/gallery/GalleryItem.vue Outdated Show resolved Hide resolved
components/gallery/GalleryItemPreviewer.vue Outdated Show resolved Hide resolved
@floyd-li @Jarsen136
Apply suggestions from code review
0e34c68 
Co-authored-by: Jarsen <31397967+Jarsen136@users.noreply.github.com>
@floyd-li
Copy link
Member Author

Mobile navbar changed

hmm seems it's the result of the break point change, i'll fix it

@codeclimate
Copy link

codeclimate bot commented Mar 24, 2023

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

View more on Code Climate.

roiLeo
roiLeo approved these changes Mar 24, 2023
View reviewed changes
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.

✅ wfm

styles/layouts/_main-navigation.scss Show resolved Hide resolved
@vikiival vikiival merged commit 6723c4d into kodadot:main Mar 24, 2023
@vikiival
Copy link
Member

pay 35 usd

@yangwao
Copy link
Member

yangwao commented Mar 24, 2023

😍 Perfect, I’ve sent the payout
💵 $35 @ 33.83 USD/KSM ~ 1.035 $KSM
🧗 EZiu1PjV2j2JHKxY6mHnFwwCRCoV27uHKQSkKXATSh1srJT
🔗 0x7e78b092805b7b57bcf5d6593558b9954f5a3cbde0ecb17305c15a54d28a54a0

🪅 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 Mar 24, 2023
@floyd-li floyd-li deleted the feat-4764 branch March 24, 2023 13:27
@floyd-li floyd-li mentioned this pull request Apr 30, 2023
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exezbcz exezbcz exezbcz left review comments

@Jarsen136 Jarsen136 Jarsen136 left review comments

@roiLeo roiLeo roiLeo approved these changes

@vikiival vikiival vikiival requested changes

Assignees
No one assigned
Labels
paid pull-request has been paid S-visual-ok-✅ S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Full screen button gallery item
7 participants
@floyd-li @kodabot @exezbcz @roiLeo @vikiival @yangwao @Jarsen136