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

redesign: [price-section] [desktop] [ui-only] buy, offer, list, transfer, change price, and not listed section #4506

Merged
merged 19 commits into from
Dec 21, 2022

Conversation

preschian
Copy link
Member

@preschian preschian commented Dec 15, 2022

Thank you for your contribution to the KodaDot NFT gallery.

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

PR Type

  • Bugfix
  • Feature
  • Refactoring
  • Redesign

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

@kodabot
Copy link
Collaborator

kodabot commented Dec 15, 2022

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

@netlify
Copy link

netlify bot commented Dec 15, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 4dd681f
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/63a044e3b25d2f00088a945b
😎 Deploy Preview https://deploy-preview-4506--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.

@roiLeo roiLeo self-requested a review December 15, 2022 08:33
@preschian preschian marked this pull request as ready for review December 16, 2022 06:19
@preschian preschian requested a review from a team as a code owner December 16, 2022 06:19
@preschian
Copy link
Member Author

ready for review cc @roiLeo. please note, this is UI only. so the button still didn't work. I put them in a separate task to integrate the button #4231 (comment). for integration, should be able to tackle in parallel PR

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.

  • From User Experience, is there any way to cancel button sliding? (initial position)
    Screenshot 2022-12-16 at 10-46-15 KodaDot - Kusama NFT Market Explorer

  • Maybe rename GalleryItemActionCta to something more meaninglfull since it can be reused on other component

  • I can't choose expiration date (maybe use radio button?)

Screenshot 2022-12-16 at 10-50-42 KodaDot - Kusama NFT Market Explorer

otherwise the rest of the code looks good to me

@preschian
Copy link
Member Author

  • From User Experience, is there any way to cancel button sliding? (initial position)

updated, using https://vueuse.org/core/onclickoutside/

  • Maybe rename GalleryItemActionCta to something more meaninglfull since it can be reused on other component

renamed cta to slides, is that ok?

  • I can't choose expiration date (maybe use radio button?)

updated using radio button

@exezbcz
Copy link
Member

exezbcz commented Dec 18, 2022

Nice job, it looks really good!

One issue, we had chat in the parent issue about the responsive version. AnnndI have to rethink it because for example from 1024 to 768px does not makes sense to have this animation etc. I did not think about it that much. I'll attempt to come up with something better - it seems to be a popup that will transform into a modal on mobile.

I mean on desktop with full view it looks really good and i am conviced this was good solution, but looking at the ui in the device toolbar i think we have to change few things - maybe use this for large screens and then change to something else, but like you said, this will result in maintaining of more than 3 types....

This was my fault and i am sorry for that.

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.

✅ code lgtm, we can continue with this

@roiLeo roiLeo added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Dec 19, 2022
@preschian
Copy link
Member Author

This was my fault and i am sorry for that.

eh, it's ok, no need to be sorry for that 👍

this will result in maintaining of more than 3 types

yes, because I already started on the desktop animation. At least deliver that first for v1.0 and adjust a little bit for mobile based on the desktop version. let's focus to deliver v1.0 first, and then we can figure it out later for v.1.1

@codeclimate
Copy link

codeclimate bot commented Dec 19, 2022

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

View more on Code Climate.

@yangwao
Copy link
Member

yangwao commented Dec 20, 2022

At least deliver that first for v1.0 and adjust a little bit for mobile based on the desktop version.

Yes, agree with this approach.
We've had a quick update on how to save some space and make mobile adjustments.
I guess we will come up with a good mobile solution and how to reuse components from mobile to desktop for example.

@yangwao
Copy link
Member

yangwao commented Dec 20, 2022

Screenshot 2022-12-16 at 10-50-42 KodaDot - Kusama NFT Market Explorer

Issue is with animation is too wide and if we account for future narrow devices, foldable, tablets we need to come up with something flexible and probably go more vertical space

To continue, we will track it in a separate mobile gallery item issue.

@exezbcz
Copy link
Member

exezbcz commented Dec 20, 2022

Hi! nice job

feedback

  • background of the card should be k-darkmode, not black
  • is it also possible to make the stroke around the frame as in design?
    image

rest i think would be fixed in #4444 , for example stretching the card to something like this, which isnt expected behavior
image

Thanks!

@preschian preschian mentioned this pull request Dec 21, 2022
17 tasks
@yangwao
Copy link
Member

yangwao commented Dec 21, 2022

I guess let's merge it and then we can look adding #4531 and then through feedback process to tune details?

@yangwao
Copy link
Member

yangwao commented Dec 21, 2022

I'm moving this forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redesign: [price-section] [desktop] [ui-only] buy, offer, list, transfer, change price, and not listed section
5 participants