joinmarket-webui / jam

Your sats. Your privacy. Your profit.
https://jamapp.org
MIT License
259 stars 53 forks source link

Deemphasise additional bond when one already exists #758

Closed barrytra closed 4 months ago

barrytra commented 4 months ago

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 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.

barrytra commented 4 months ago

Fixes in new commits

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 commented 4 months ago

From our conversation:

barrytra commented 4 months ago

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

theborakompanioni commented 4 months ago

Pls have a review @theborakompanioni

Looks nice and exactly as discussed. :rocket:

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 commented 4 months ago

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 commented 4 months ago

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 :muscle: