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

feat: transfer redesign for mobile #6764

Merged
merged 10 commits into from
Aug 20, 2023
Merged

feat: transfer redesign for mobile #6764

merged 10 commits into from
Aug 20, 2023

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Aug 17, 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

  • Feature
  • Refactoring

Needs Design check

  • @exezbcz please review
  • @kodadot/qa-guild please review

Context

Did your issue had any of the "$" label on it?

Screenshot 📸

  • My fix has changed UI
image image

Copilot Summary

🤖 Generated by Copilot at 0bdf37f

Improved the responsiveness and design of the transfer feature for NFTs. Refactored the Transfer and TransferConfirmModal components and added flex layout and mobile detection. Enhanced the NeoModal component to accept multiple custom classes. Removed the old transfer design from the TransferPage component.

🤖 Generated by Copilot at 0bdf37f

We are the masters of the NFT
We transfer them with style and speed
We adapt to any device or screen
We are the kings of the Transfer component

@Jarsen136 Jarsen136 requested a review from a team as a code owner August 17, 2023 15:56
@Jarsen136 Jarsen136 requested review from preschian and roiLeo and removed request for a team August 17, 2023 15:56
@kodabot
Copy link
Collaborator

kodabot commented Aug 17, 2023

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

@netlify
Copy link

netlify bot commented Aug 17, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit c57ab6d
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64df6c73a99d3e00088721a6
😎 Deploy Preview https://deploy-preview-6764--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 configuration.

@kodabot
Copy link
Collaborator

kodabot commented Aug 17, 2023

SUCCESS @Jarsen136 PR for issue #6468 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@reviewpad
Copy link
Contributor

reviewpad bot commented Aug 17, 2023

AI-Generated Summary: This pull request reflects significant alterations in several files which aim to improve mobile responsiveness and amend layout requirements, primarily focusing on the transfer components.

The file 'TransferOld.vue', used for fund transfers functionality, has been completely removed, necessitating a refactoring of these features into another section if still required. Meanwhile, 'NeoModal.vue' has seen adjustable CSS classes introduced to add versatility to modal content displays.

The 'Transfer.vue' file underwent modifications for mobile-friendly adjustments along with the addition of mobile-related attributes/classes. There has also been a noticeable redesign of the transfer component and removal of references to 'TransferOld'.

In 'TransferConfirmModal.vue', additional mobile property was introduced along with modifications in styles, class assignments, and the handling of mobile views. Lastly, in 'transfer.vue', redundancies were removed, alterations were made in layout and scripts, and the older design was replaced with a newer, more definitive version. Overall, these changes seem to greatly improve mobile responsiveness and usability.

@reviewpad reviewpad bot added large Pull request is large waiting-for-review labels Aug 17, 2023
@prury
Copy link
Member

prury commented Aug 17, 2023

looks nice! let's go

i believe this button doesn't have this square around it
image

4 recipients but showing only 2 (they show if you scroll down, but there's a blank/dark space over them)
image

will leave design details for Exez

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Aug 17, 2023
@yangwao
Copy link
Member

yangwao commented Aug 17, 2023

Screenshot_20230817_194123_GitHub.jpg

Koda has own song 🎶 now

@exezbcz
Copy link
Member

exezbcz commented Aug 17, 2023

i believe this button doesn't have this square around it

We can either remove completely or just remove the drop shadow - general rule - it is in container that has drop shadow -> no drop shadow on elements inside

@Jarsen136
Copy link
Contributor Author

4 recipients but showing only 2 (they show if you scroll down, but there's a blank/dark space over them)

✅ Fixed

i believe this button doesn't have this square around it
We can either remove completely or just remove the drop shadow - general rule - it is in container that has drop shadow -> no drop shadow on elements inside

✅ Done

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.

You really like <span /> hehe

libs/ui/src/components/NeoModal/NeoModal.vue Show resolved Hide resolved
components/transfer/TransferConfirmModal.vue Outdated Show resolved Hide resolved
@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 Aug 18, 2023
@yangwao
Copy link
Member

yangwao commented Aug 18, 2023

so I'm dispatching payrolls with new UI ? 👀

@exezbcz
Copy link
Member

exezbcz commented Aug 18, 2023

Hellooooo
image
remove stroke on sides

  • modal background full height, even covering the navbar

  • is there an available fund check? I should not be able to send this transaction
    image

otherwise good! thanks

@yangwao
Copy link
Member

yangwao commented Aug 18, 2023

hmm, usd missing?

image

@codeclimate
Copy link

codeclimate bot commented Aug 18, 2023

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

View more on Code Climate.

@sonarcloud
Copy link

sonarcloud bot commented Aug 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.5% 1.5% Duplication

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Aug 18, 2023

is there an available fund check? I should not be able to send this transaction

hmm, usd missing?

image

✅ Fixed. The bug comes from some recent merging : )

@Jarsen136
Copy link
Contributor Author

remove stroke on sides

✅ Done

  • modal background full height, even covering the navbar

updated

I cannot manage to make some elements cover navbar for the current layout. How about a higher mobile modal?

image

@yangwao
Copy link
Member

yangwao commented Aug 19, 2023

How about a higher mobile modal?

for me yes
cc @exezbcz

@exezbcz
Copy link
Member

exezbcz commented Aug 19, 2023

@Jarsen136 I think that does not solve the issue; I would not make it that high, maybe a bit smaller, and it will work - it is the default modal height or?

thanks!

cc @EthVlad

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Aug 19, 2023

@Jarsen136 I think that does not solve the issue; I would not make it that high, maybe a bit smaller, and it will work - it is the default modal height or?

thanks!

The default max height of NeoModal component is 80% of the viewport. Actually, we do not have a common modal style for mobile device. Could you tell me what kind of issue you are concerned about based on the current design?

image

Current implementation
image

image

@exezbcz
Copy link
Member

exezbcz commented Aug 19, 2023

@Jarsen136 okay good, we can always change it, thanks!

@yangwao
Copy link
Member

yangwao commented Aug 20, 2023

pay 100 usd

let's go!

@yangwao yangwao merged commit 348333f into kodadot:main Aug 20, 2023
14 checks passed
@yangwao
Copy link
Member

yangwao commented Aug 20, 2023

😍 Perfect, I’ve sent the payout
💵 $100 @ 4.52 USD/DOT ~ 22.124 $DOT
🧗 16SjUbGKSdjCdWTy3NNT3JxbRVGGqD4mwkHpc6BD9U2Rp29Z
🔗 0x382b67cebc16c43673460a925c5d86cb5dc5a451f9a3b9f10a542d9a6b8a5baa

🪅 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 Aug 20, 2023
This was referenced Aug 20, 2023
@EthVlad
Copy link

EthVlad commented Aug 21, 2023

I cannot manage to make some elements cover navbar for the current layout. How about a higher mobile modal?

What do you mean by higher mobile modal? I think that covering 80% is good enough if we could darken the navbar same way as the background. Like here in design, so we focus users attention to modal only. Also user should not be able to access the navbar here.

He could only tap outside the modal to close it but not trigger any other action. User can close is therefore 3 ways: tap outside modal, tap close button, maybe swipe down the modal with their finger (if possible).
Screenshot 2023-08-21 at 10 45 32

@yangwao
Copy link
Member

yangwao commented Aug 21, 2023

so I'm dispatching payrolls with new UI ? 👀

the answer is no because we have missing dialogue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large 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 waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer Redesign handover - (4) Mobile Redesign Transfer Redesign handover
8 participants