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.

Sign up for GitHub

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

Jump to bottom

fix(BackUpSeed): Confirm # word input had no focus #7958

Merged
merged 1 commit into from
Oct 24, 2022
+14 −0
Merged

fix(BackUpSeed): Confirm # word input had no focus #7958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ui/app/AppLayouts/Profile/popups/BackupSeedModal.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ StatusStackModal {

property var privacyStore

onCurrentIndexChanged: {
//StatusAnimatedStack doesn't handle well items' visibility,
//keeping this solution for now until #8024 is fixed
if (currentIndex === 2) {
Copy link
Member

Choose a reason for hiding this comment

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

My proposition is to put it directly to components:

BackupSeedStepBase {
            id: confirmFirstWord
            ...
            onVisibleChanged: if (visible) forceInputFocus()
}

to avoid comparing to index numbers and better fulfill solid's open-close principle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @micieslak visibility in StackLayout items is not working this way unless it's explicitly set like visible: stack.currentIndex === 1, visible: stack.currentIndex === 2 etc . so it's kind of the same thing - the current index should be checked and compared. See video for credibility :D
https://user-images.githubusercontent.com/31625338/196982590-8daf01f8-011a-488a-835d-4c7352fe4116.mov

Copy link
Member

Choose a reason for hiding this comment

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

Hey @alexandraB99. I found the source of misunderstanding :) StackLayout works exactly this way - current item is visible, the rest is not visible. But... we are not using raw StackLayout. We are using StatusAnimatedStack which is buggy and sets visible in a strange way, exactly as you presented (I was able to reproduce that as well). On this movie I print visibility after temporarily changing to plain StackLayout:

Kazam_screencast_00071.webm

Probably StatusAnimatedStack should have been based on StackView which has build-in support for animated transitions, but ofc it's a different story.
So I agree on current solution, but I would at least add some comment about the reason why it's done this way, and, ideally, create a new ticket for that bug in StatusAnimatedStack. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micieslak comment added and issue, there you go: #8024

confirmFirstWord.forceInputFocus();
} else if (currentIndex === 3) {
confirmSecondWord.forceInputFocus();
}
}

QtObject {
id: d

Expand Down
4 changes: 4 additions & 0 deletions ui/app/AppLayouts/Profile/popups/backupseed/BackupSeedStepBase.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ StatusScrollView {

default property alias content: column.children

function forceInputFocus() {
inputText.input.edit.forceActiveFocus();
}

implicitHeight: 520
clip: false

Expand Down