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

Deemphasise additional bond when one already exists #758

Merged

Conversation

barrytra
Copy link
Contributor

This PR fixes #756
Size of the button has been reduced to look like this:
Screenshot from 2024-05-16 09-44-44

Earlier it also had a message displayed as : "If more than one fidelity bond exists, only the <0>the most valuable one</0> will be used." I don't get how to use this if we reduce the button size. So for now I have just commented this portion. One Idea is simply we can add this info in settings which will be tackled in next PR. Would love to hear any other suggestion as well.

Secondly the dropdown now looks a bit weird. like this:
Screenshot from 2024-05-16 09-50-05
But This eventually should not be a problem as we would have a modal instead of dropdown.

@theborakompanioni theborakompanioni added UI/UX Issue related to cosmetics, design, or user experience Fidelity Bonds Label for grouping FB issues labels May 16, 2024
border: 1px solid var(--bs-gray-200);
border-radius: 0.3rem;
display: block;
margin-left: 41rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of positioning with margins, what do think about using flexbox?
e.g. you could use d-flex justify-content-end on the parent element to position the button to the "right".

@@ -617,7 +617,7 @@ const CreateFidelityBond = ({ otherFidelityBondExists, wallet, walletInfo, onDon
}

return (
<div className={styles.container}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

On first glance, it seems the design has slightly changed, since the container css class moved away from here.

Before

After

Dropdown content is not within the surrounding area border anymore (no FB present).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes the code changes will make this issue. I kinda knew it. But If I try to fix then <rb.collapse> section will have to be repeated twice for both the cases ( active bond exists and no active bond). This will just increase the code length.
One solution is that eventually it will be a modal so this issue shouldn't be there after we do that. So we can possibly fix that issue(modal instead of dropdown for the form) first and then there will be nothing to talk about this.

<a
onClick={(e) => e.stopPropagation()}
{otherFidelityBondExists ? (
<div className={styles.containerWhenBondAlreadyExists}>
Copy link
Collaborator

@theborakompanioni theborakompanioni May 16, 2024

Choose a reason for hiding this comment

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

I think it can be achieved without (or with less) new css classes:

<div className="d-flex justify-content-end">
  <rb.Button size="sm" variant="outline-dark" onClick={() => setIsExpanded(it => !it)}>
    <div className="d-flex justify-content-center align-items-center">
      {t('earn.fidelity_bond.title_fidelity_bond_exists')}
      <Sprite className="ms-1" symbol={isExpanded ? 'caret-up' : 'plus'} width="16" height="16" />
    </div>
  </rb.Button>
</div>

What do you think?

<Sprite symbol={isExpanded ? 'caret-up' : 'plus'} width="15" height="15" />
</div>
{/* <div className={styles.subtitle}>
<Trans i18nKey="earn.fidelity_bond.subtitle_fidelity_bond_exists">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have an idea where we can display the message? Maybe after the users actively clicks the button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can show this message at the top of the form when user clicks the button, only in the case when active bonds exist.

@barrytra
Copy link
Contributor Author

barrytra commented May 24, 2024

Fixes in new commits

  • when there doesn't exist any bond earlier UI looks like this:
    Screenshot from 2024-05-24 16-50-49
  • When FB already exists, UI is like:
    Screenshot from 2024-05-24 16-51-24
  • When FB already exists, the message If more than one fidelity bond exists, only the <0>the most valuable one</0> will be used. is displayed like this:
    Screenshot from 2024-05-24 16-51-39

Suggestions needed

  1. The plus symbol in Configure additional bond button (when bond already exists) doesn't look very attractive to me. We could possibly remove that and IMO it won't change any significance.
  2. Please look up on CSS part. i have tried to keep it minimal. Still not sure if more simplification is possible.

please have a look @theborakompanioni

@theborakompanioni
Copy link
Collaborator

From our conversation:

  • The button should be centered
  • The button should be a real button element
  • Icon should be in front of the label with a little bit of spacing (e.g. like the "Show orderbook" button)
  • Button should be slightly smaller (use <rb.Button size="sm" ... />)

@barrytra
Copy link
Contributor Author

barrytra commented Jun 4, 2024

Suggested changes have been made..
Screenshot from 2024-06-04 20-35-07
Pls have a review @theborakompanioni

@theborakompanioni
Copy link
Collaborator

Pls have a review @theborakompanioni

Looks nice and exactly as discussed. 🚀

Can you check whether some added css classes are unused and can be removed?

Also, what do you think.. can we bring the existing bond help text back somehow?

<Trans i18nKey="earn.fidelity_bond.subtitle_fidelity_bond_exists">
              <a
                onClick={(e) => e.stopPropagation()}
                rel="noopener noreferrer"
                href="https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/docs/fidelity-bonds.md#what-amount-of-bitcoins-to-lock-up-and-for-how-long"
              >
                {/* i18n placeholder */}
              </a>
            </Trans>

@barrytra
Copy link
Contributor Author

barrytra commented Jun 4, 2024

Also, what do you think.. can we bring the existing bond help text back somehow?

<Trans i18nKey="earn.fidelity_bond.subtitle_fidelity_bond_exists">
              <a
                onClick={(e) => e.stopPropagation()}
                rel="noopener noreferrer"
                href="https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/docs/fidelity-bonds.md#what-amount-of-bitcoins-to-lock-up-and-for-how-long"
              >
                {/* i18n placeholder */}
              </a>
            </Trans>

ya my fault.. It somehow disappeared from my code. Added it back now.. and css file has also been omitted.

@theborakompanioni
Copy link
Collaborator

Also, what do you think.. can we bring the existing bond help text back somehow?

<Trans i18nKey="earn.fidelity_bond.subtitle_fidelity_bond_exists">
              <a
                onClick={(e) => e.stopPropagation()}
                rel="noopener noreferrer"
                href="https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/docs/fidelity-bonds.md#what-amount-of-bitcoins-to-lock-up-and-for-how-long"
              >
                {/* i18n placeholder */}
              </a>
            </Trans>

ya my fault.. It somehow disappeared from my code. Added it back now.. and css file has also been omitted.

Nice. Looks good!

There really should be little to no reason for users to have more than one active fidelity bond, so I think this message can be emphasized a bit more. But I am okay with this being tackled in a follow-up PR.

Great work @barrytra - way better than before 💪

@theborakompanioni theborakompanioni self-requested a review June 4, 2024 17:00
@barrytra barrytra merged commit 9ec7f14 into joinmarket-webui:master Jun 5, 2024
3 checks passed
@barrytra barrytra deleted the deemphasise_additional_bond branch June 5, 2024 08:14
@barrytra barrytra restored the deemphasise_additional_bond branch June 5, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fidelity Bonds Label for grouping FB issues UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

De-emphasize creating a second fidelity bond (if non-expired bond already exists)
2 participants