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

Addressbook restyle #1741

Merged
66 commits merged into from
Jun 15, 2022
Merged

Addressbook restyle #1741

66 commits merged into from
Jun 15, 2022

Conversation

ghost
Copy link

@ghost ghost commented Apr 15, 2022

Closes #1464
Closes #1770

@ghost ghost marked this pull request as ready for review May 9, 2022 11:43
@ghost ghost requested review from Canialon and smk762 May 11, 2022 17:30
@tonymorony
Copy link

@smk762 @Canialon please give this PR one more testing session :)

@tonymorony tonymorony self-requested a review June 6, 2022 12:03
@smk762
Copy link
Collaborator

smk762 commented Jun 7, 2022

Need little more spacing under the combobox. too much spacing under address input
image

expanded box perhaps too big when only a few coins to select. smaller font for "use standard network address" so it fits on 2 lines.
image

Error message could be reduced to omit the bits before the second ]. When error is 2 lines, buttons almost outside box.
Error text persists when adding second address after successful entry.
image

Unable to select coins that are not enabled (optional fr: make like bestorders with fade and option to enable on click). If QTUM not enabled, I can still attempt to add a QRC-20 address, but will get a validation error.
image

After confirming entries, main screen shows "no registered addresses". This does not update until going to other tab (e.g. wallet) and then back.
https://user-images.githubusercontent.com/35845239/172281479-8c5a1938-5327-4147-bd5e-06ce202e32a8.mp4

Might be good to have sight different background for the contact address list area (example below):
image

Addressbook entries not visible after restarting app. Seems cause of this was wallet name existing as folder (from previous builds, folder was datestamped almost 1 year ago). Not a problem for this PR, but noting for potential future support queries.

[10:22:52] [warning] [addressbook.cfg.cpp:48] [2565789]: Addressbook config file was invalid, use empty configuration: [json.exception.parse_error.101] parse error at line 1, column 1: syntax error while parsing value - unexpected end of input; expected '[', '{', or a literal. Content was: 

After deleting folder, everything fine - new file created, entries visibile on subsequent starts.

@ghost
Copy link
Author

ghost commented Jun 8, 2022

Error message could be reduced to omit the bits before the second ]. When error is 2 lines, buttons almost outside box.
Error text persists when adding second address after successful entry.
image

I'm sure this one also occurs in the send modal or in every other part where we need to validate an address: we should make an issue to sanitize those error messages

@smk762
Copy link
Collaborator

smk762 commented Jun 9, 2022

@ghost
Copy link
Author

ghost commented Jun 13, 2022

A lot of stuff related to asset network standards have been hardcoded in the frontend in several places. When we will add a new one it will not be so trivial.

This brings us to something we will need to implement in next patches: some models (e.g. coins model) in the backend are poorly designed for our current needs and some are still missing (e.g. network standard model)

@ghost ghost requested a review from smk762 June 13, 2022 09:40
@smk762
Copy link
Collaborator

smk762 commented Jun 13, 2022

Seen when clicking "edit" for a contact.
[19:51:29] [warning] [main.prerequisites.hpp:92] [2083955]: qrc:/Dex/Addressbook/AddAddressForm.qml:114:13: QML AddressTypeSelector: Binding loop detected for property "implicitHeight"

When adding coin/address entry, there is no "default selection" in dropdown (is blank)
image

A little overlap detected here
image

"Enable custom fees" text seems to have gone missing. Also happens when sending from wallet page.
image

Copy link
Collaborator

@smk762 smk762 left a comment

Choose a reason for hiding this comment

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

Looking good thanks :)

@smk762 smk762 requested a review from cipig June 15, 2022 03:10
@ghost ghost merged commit c2929de into dev Jun 15, 2022
cipig added a commit to cipig/atomicDEX-Desktop that referenced this pull request Jun 15, 2022
…dressbook_restyle"

This reverts commit c2929de, reversing
changes made to c5ab16c.
@tonymorony tonymorony mentioned this pull request Jun 21, 2022
@ghost ghost deleted the addressbook_restyle branch September 15, 2022 10:14
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addressbook restyle tweaks 🎉 [FEATURE REQUEST]: Addressbook enhancement
3 participants