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

gui(key modal): make the list of keys/signers scrollable #1426

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pythcoiner
Copy link
Collaborator

this is a variant of #1412, it make only signer/key list scrollable instead the whole modal
fixes #1402

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Nov 4, 2024

When first opening the modal, there's a lot empty space below the keys. Is it possible to remove that?

image

@pythcoiner
Copy link
Collaborator Author

pythcoiner commented Nov 4, 2024

hum, the issue is if we define a scrollable w/o height or with Length::Shrink, it will not be scrollable 🥲

One solution is to put a fixed height for the scrollable:

diff --git a/gui/src/installer/view/editor/mod.rs b/gui/src/installer/view/editor/mod.rs
index 3b7bdf9..b3fe63a 100644
--- a/gui/src/installer/view/editor/mod.rs
+++ b/gui/src/installer/view/editor/mod.rs
@@ -327,7 +327,7 @@ pub fn edit_key_modal<'a>(
         )
             .padding(15)
     ;
-    let scroll = scrollable(key_column).height(Length::Fill);
+    let scroll = scrollable(key_column).height(300);
     Column::new()
         .padding(25)
         .push_maybe(error.map(|e| card::error("Failed to import xpub", e.to_string())))

(we can modify this fixed height dynamically, but we need to get the window height, and iirc, should add a subscription to catch the signal...)

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Nov 4, 2024

Perhaps you could you set the height to Length::Fill only if there's at least one non-hot key?

@pythcoiner
Copy link
Collaborator Author

Perhaps you could you set the height to Length::Fill only if there's at least one non-hot key?

done!

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Nov 4, 2024

Unfortunately there's still a problem when an xpub has been entered manually:

image

Maybe this can also be taken into account in the keys length. Or perhaps we can do something simpler for now, such as the fixed height approach you suggested above.

@pythcoiner
Copy link
Collaborator Author

i reduced the number of key to > 1 for trigger the scrollable

@pythcoiner
Copy link
Collaborator Author

or i can put the Key name card in the scrollable and trigger the scroll if more than 3 items into, but i think w/ many keys, user will not see Key name card appear, as it will be in the hidden part of the scrollable....

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Nov 4, 2024

i reduced the number of key to > 1 for trigger the scrollable

The problem still occurs with this condition. It doesn't seem to occur with > 0, but then this gives a lot of empty space when the window is large. Perhaps the fixed height approach would be better? Or otherwise #1412 for now.

or i can put the Key name card in the scrollable and trigger the scroll if more than 3 items into, but i think w/ many keys, user will not see Key name card appear, as it will be in the hidden part of the scrollable....

I agree it's nicer to have the scrollable part only including the keys.

@pythcoiner
Copy link
Collaborator Author

pythcoiner commented Nov 4, 2024

@jp1ac4 wdyt about the last change? (i think it can be better if we auto scroll down when manual inserting an xpub & window minimised - a lot of conditions 😄 - , but it's maybe a bit tricky)

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Nov 4, 2024

Hmm, there's still this problem with having a lot of empty space when the window is large. Even though having only the keys scrollable is preferable, I would go with #1412 for now until we find a better way to manage the height.

@nondiremanuel
Copy link
Collaborator

I agree with going with the simpler solution for now. I will move it to the following milestone in case we want to investigate more on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI gui related
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Installer - "Apply" button sometimes not filled when setting up a key
3 participants