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

feat: payment confirm modal #446

Merged
6 commits merged into from
Aug 4, 2022
Merged

feat: payment confirm modal #446

6 commits merged into from
Aug 4, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Aug 4, 2022

Closes #385.

Before this PR a payment was initiated immediately after pressing the submit button.
When this PR is applied, a user has the chance to review the given values and possibly catch some input errors before the payment is initiated.

Any improvements or suggestions are welcome.
(e.g. Would it be better to move the privacy info text to the title bar instead of having an own space in the modal body?)

Without privacy improvement

With privacy improvement

Sweep (with privacy improvement)

@theborakompanioni theborakompanioni self-assigned this Aug 4, 2022
@theborakompanioni theborakompanioni added enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience labels Aug 4, 2022
@theborakompanioni theborakompanioni marked this pull request as ready for review August 4, 2022 10:40
@theborakompanioni theborakompanioni requested a review from a user August 4, 2022 10:40
@ghost
Copy link

ghost commented Aug 4, 2022

Quick UI improvement: Move the info icon to the right and make it a bit smaller. Looks more symmetrical. I'd also drop the "approx." which sounds it a bit confusing imo.

Screenshot 2022-08-04 at 14 52 40

Here's the diff:

diff --git a/src/components/Send.jsx b/src/components/Send.jsx
index 2ee7925..4ec43a9 100644
--- a/src/components/Send.jsx
+++ b/src/components/Send.jsx
@@ -969,36 +969,32 @@ export default function Send() {
             </rb.Row>
             <rb.Row>
               <rb.Col xs={3} className="text-end">
-                {isSweep ? (
-                  <rb.OverlayTrigger
-                    placement="right"
-                    overlay={
-                      <rb.Popover>
-                        <rb.Popover.Body>{t('send.confirm_modal.text_sweep_info_popover')}</rb.Popover.Body>
-                      </rb.Popover>
-                    }
-                  >
-                    <div className="d-inline-flex align-items-center">
-                      <strong>{t('send.confirm_modal.label_amount')}</strong>
-                      <span className="ms-1">
-                        <Sprite className={styles.infoIcon} symbol="info" width="18" height="18" />
-                      </span>
-                    </div>
-                  </rb.OverlayTrigger>
-                ) : (
-                  <strong>{t('send.confirm_modal.label_amount')}</strong>
-                )}
+                <strong>{t('send.confirm_modal.label_amount')}</strong>
               </rb.Col>
               <rb.Col xs={9} className="text-start">
                 {isSweep ? (
-                  <Trans i18nKey="send.confirm_modal.text_sweep_balance">
-                    Sweep
-                    <Balance
-                      valueString={amountFieldValue().toString()}
-                      convertToUnit={settings.unit}
-                      showBalance={true}
-                    />
-                  </Trans>
+                  <div className="d-flex justify-content-start align-items-center">
+                    <Trans i18nKey="send.confirm_modal.text_sweep_balance">
+                      Sweep
+                      <Balance
+                        valueString={amountFieldValue().toString()}
+                        convertToUnit={settings.unit}
+                        showBalance={true}
+                      />
+                    </Trans>
+                    <rb.OverlayTrigger
+                      placement="right"
+                      overlay={
+                        <rb.Popover>
+                          <rb.Popover.Body>{t('send.confirm_modal.text_sweep_info_popover')}</rb.Popover.Body>
+                        </rb.Popover>
+                      }
+                    >
+                      <div className="d-inline-flex align-items-center">
+                        <Sprite className={styles.infoIcon} symbol="info" width="13" height="13" />
+                      </div>
+                    </rb.OverlayTrigger>
+                  </div>
                 ) : (
                   <Balance
                     valueString={amountFieldValue().toString()}
diff --git a/src/components/Send.module.css b/src/components/Send.module.css
index e18ce89..4026b83 100644
--- a/src/components/Send.module.css
+++ b/src/components/Send.module.css
@@ -151,6 +151,7 @@ input[type='number'] {
 }
 
 .infoIcon {
+  margin: 2px 0 0 0.25rem;
   color: var(--bs-gray-500);
   border: 1px solid var(--bs-gray-500);
   border-radius: 50%;
diff --git a/src/i18n/locales/en/translation.json b/src/i18n/locales/en/translation.json
index 2b68f62..a1d3a69 100644
--- a/src/i18n/locales/en/translation.json
+++ b/src/i18n/locales/en/translation.json
@@ -191,7 +191,7 @@
     "confirm_modal": {
       "title": "Confirm payment",
       "label_amount": "Amount",
-      "text_sweep_balance": "Sweep whole jar (approx. <1></1>)",
+      "text_sweep_balance": "Sweep whole jar (<1></1>)",
       "text_sweep_info_popover": "The exact transaction amount can only be calculated by JoinMarket at the point when the transaction is made.",
       "label_recipient": "Recipient",
       "label_source_jar": "Send from",

@theborakompanioni
Copy link
Collaborator Author

Quick UI improvement: Move the info icon to the right and make it a bit smaller. Looks more symmetrical. I'd also drop the "approx." which sounds it a bit confusing imo.

Agreed!

Here's the diff: [...]

Nice, thank you. Applied! Much better now.

@ghost ghost merged commit 29eca37 into master Aug 4, 2022
@ghost ghost deleted the confirm-send branch August 4, 2022 14:26
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Q: Display confirm modal before initiating transaction?
1 participant