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

Add SPACE, DEL icons to Keyboard; various UI consistency tweaks #664

Merged
merged 5 commits into from
Jan 28, 2025

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Jan 19, 2025

Description

See affected screenshot changes: SeedSigner/seedsigner-screenshots#5

UPDATE: 2025-01-23:

  1. Per @easyuxd's guidelines, tightened up the icon + color usage (7edcd33):

    There should be only 3 combinations for warnings and errors:

    • Warning: WARNING / WARNING_COLOR (Yellow)
    • Dire Warning: WARNING / DIRE_WARNING_COLOR (Orange)
    • Error: ERROR / ERROR_COLOR (Red)

Note that the goal was to enforce his consistency rules, but I have not implemented his guidelines for when to use which warning/error. I think there are deeper discussions we need to have about what an error means and/or how it is perceived by the user (preview: I think "error" implies something is broken in our code and thus we need to take great care when telling the user there was an error).

  1. And in order to begin to get a better handle on consistently applying those rules, I finally implemented ScanInvalidQRTypeView (c0fe676) which had previously been a manually-created View that the screenshot generator had to also manually create to provide the matching screenshot. Note the comment/misgivings about its "Error" title.

  2. Misc trivial import cleanup while I was in those files.


Main purpose:

UI fixes:

  • We had never used the ERROR icon (we were using the WARNING exclamation in red instead).

UI consistency tweaks:

  • Slightly opinionated edits for when to use ERROR vs WARNING and red vs yellow.
  • General principles applied:
    • Red = Extreme danger (displaying mnemonic, PSBT addr verification failed, etc)
    • Red = System error, bug, showstopper
    • Yellow = Warning; be aware but don't scare the user.
    • Yellow = Incompatibility (e.g. unknown QR format). Yes, something went wrong, but don't scare the user.
  • Judgment call: Signing PSBT didn't produce a valid signature: I used WARNING in yellow.
    • Rationale: maybe they already signed the multisig PSBT with this key. That's not a reason to panic. Or they opted to try to sign with a key that we didn't think could sign in the first place (has the "(?)" on the selection screen).

Minor misc:

  • Keyboard: Use our CHEVRON_LEFT / CHEVRON_RIGHT for cursor navigation.
  • I/O Test Screen: Use our icons instead of FontAwesome.
  • Remove legacy load_icon() function which was an early ~v0.5.0 relic.
  • Minor spacing fix at the bottom edge of ButtonListScreen where is_bottom_list=True with more than one button.
  • Render Dice icons to fill the Keyboard key size.
  • Change to text when multisig self-transfer addr can't be verified by descriptor; edited to fit better in available space. Text change affects translations (messages.pot).
  • Applied "Suspicious PSBT" consistently across single sig and multisig.
  • Screenshot generator tweaked to generate same results as the ScanView error when it scans an unsupported QR format.

This pull request is categorized as a:

  • UI cleanup

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • N/A

I have tested this PR on the following platforms/os:

@jdlcdl
Copy link

jdlcdl commented Jan 19, 2025

As of 5c6c99b

This is working for me. Have reviewed code changes, all reasonable to me.
Have tested a little, but will test more, on pi0 built by seedsigner-os pr-83
Have carefully reviewed screenshots in english, compared to merges made mid-last week, all as expected given the code changes.

@newtonick
Copy link
Collaborator

ACK the delete and space icon changes.

I believe the inconsistencies that @easyuxd has pointed out with https://github.com/easyuxd/seedsigner-ux-guidelines?tab=readme-ov-file#message-types should be addressed is the changes to error/warning icons are kept in this PR.

@newtonick newtonick added this to the 0.8.5 milestone Jan 23, 2025
@newtonick newtonick added the gui label Jan 23, 2025
@kdmukai kdmukai force-pushed the keyboard_space_del_icons branch from 5c6c99b to 5e16c98 Compare January 23, 2025 14:38
@newtonick
Copy link
Collaborator

Tested and reviewed

@newtonick newtonick merged commit b99df74 into SeedSigner:dev Jan 28, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Merged Not Yet Released
Development

Successfully merging this pull request may close these issues.

3 participants