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

Ask before opening external links #343

Merged
merged 2 commits into from
Jan 14, 2024

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Jun 3, 2023

Introduces an external link confirm popup window that prompts the user to acknowledge that they really want to visit the external link. This is based off the design file

This contains a menu bar with a title, but is missing the close button. It is also missing the "Don't ask again" button. These require some more conversation and can be addressed in a follow-up.

This deviates from the design file in providing a mobile layout that positions the buttons in a column layout instead of a rowlayout. cc @GBKS @mouxdesign

Desktop Layout Mobile Layout
Screen Shot 2023-06-03 at 2 11 30 AM Screen Shot 2023-06-03 at 2 12 43 AM

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android
ARM32 Android

@jarolrod jarolrod added enhancement New feature or request Needs design review Designer's review needed Design suggestion Makes a suggestion on design labels Jun 3, 2023
@jarolrod
Copy link
Member Author

jarolrod commented Jun 3, 2023

Updated from 3a3f8c2 to e9b6d4a

Added a navigation bar with a Title, but without a close button within the navigation bar.

@thebagmaster
Copy link

ACK e9b6d4a
Verified on Ryzen 9 Ubuntu 22.04.2 and Note 9 Android 10 on fully synced nodes.

@jarolrod jarolrod added this to the v1.1 milestone Jun 6, 2023
Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

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

tested ACK on Galaxy A13 5G (armv7a) built on WSL Ubuntu 22.04
PR built w/o issues.

nit, the “External Link” text header seems to be a little off center in portrait mode (in landscape it looks centered) see attachment

The adjustment is a mock up made with image manipulation software, no code was altered.
QML_PR_343_sideComp

@GBKS
Copy link
Contributor

GBKS commented Jun 28, 2023

Good idea with the column layout for mobile. Small thing, the overlay should have a 1px border around it (neutral 6 color).

Layout.fillWidth: true
header: qsTr("External Link")
headerBold: true
headerSize: 24
Copy link
Contributor

Choose a reason for hiding this comment

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

center: true // Center header text

@GBKS
Copy link
Contributor

GBKS commented Jul 10, 2023

@jarolrod pointed out that this should probably be an application-wide setting. So the first time a user opens an external link, we ask them. If they check the "Don't ask again" option, we don't ask anymore. But they can change this behavior again in the settings. Here's a mock-up for that.

image

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tested ACK on desktop built on both Ubuntus 20.04 & 22.04.

As pointed out by @D33r-Gee, the title of the popup "External Link" is not properly centered (it didn't look right at first sight and checked it with a screen ruler - KRuler, perhaps there's a better tooling), try adding center: true in middleDetail: Header block and re-compiling didn't make any difference for me (event setting it to false doesn't). What reduced the pixels difference (~6 vs ~10 - but to the right direction instead) is adding Layout.alignment: Qt.AlignHCenter in NavigationBar block in ExternalPopup.qml but it's like duplicating what's already specify in NavigationBar.qml (id: middle_detail):

image

image image

I like the "Don't ask again" toggle switch proposed by @jarolrod and mocked-up by @GBKS.

@promag
Copy link
Contributor

promag commented Aug 31, 2023

Concept ACK, approach NACK.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

I've rebuilt this PR on top of #358 using NavBar2 instead and the title of the popup is now more accurately centered.

image image

@hebasto
Copy link
Member

hebasto commented Aug 31, 2023

All usage of this defines the width. This makes the buttons usable in a
wider range of situations. We keep the implicitHeight.
@jarolrod
Copy link
Member Author

jarolrod commented Sep 1, 2023

rebased over main

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tack 726bfa2, works as expected

@GBKS
Copy link
Contributor

GBKS commented Jan 11, 2024

This issue has been around for a long time. What is needed to wrap it up?

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

re ACK 726bfa2

@hebasto hebasto merged commit be965bf into bitcoin-core:main Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design suggestion Makes a suggestion on design enhancement New feature or request Needs design review Designer's review needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants