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(core, extension): add balance style to the dapp bridge [LW-9887] #943

Conversation

DominikGuzei
Copy link
Member

@DominikGuzei DominikGuzei commented Mar 7, 2024

Checklist


Proposed solution

There has been a small style change in the wallet activity to differentiate operations that are negative or positive so users don’t have any doubt about the balance change in their wallet. This task is about bringing the same styling to the dapp bridge.

Testing

Try various DApp transactions and check that:

  • All positive amounts are rendered in green (without a + sign) - except for collateral which is always displayed in default style
  • Items that are not present in the Tx, or with value 0, should not be displayed (collateral, deposit)
  • font sizes are unified for all tx summary amounts

Screenshots

Bildschirmfoto 2024-03-20 um 12 42 26

@DominikGuzei DominikGuzei self-assigned this Mar 7, 2024
@DominikGuzei DominikGuzei requested a review from a team as a code owner March 7, 2024 11:47
@DominikGuzei DominikGuzei added browser Changes to the browser application. ui-toolkit labels Mar 7, 2024
@coveralls
Copy link

coveralls commented Mar 7, 2024

Coverage Status

Changes unknown
when pulling d1cb93d on feat/lw-9887-add-balance-style-to-the-dapp-bridge
into ** on feat/lw-9145-tx-summary-refactor**.

@VanessaPC VanessaPC self-requested a review March 7, 2024 13:29
@@ -144,9 +145,9 @@ export const DappTransaction = ({
))}
</div>

{collateral && (
{collateral !== BigInt(0) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I see collateral is optional. Does this condition check prevents us from displaying when collateral is undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, will add the additional check 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 82c5116c3fe64d219d7edd351298901aeabf6056

@DominikGuzei DominikGuzei force-pushed the feat/lw-9887-add-balance-style-to-the-dapp-bridge branch from 7c55b60 to 9fe2fcb Compare March 8, 2024 11:26
Copy link

github-actions bot commented Mar 8, 2024

Allure report

allure-report-publisher generated test report!

smokeTests: ❌ test report for d1cb93db

passed failed skipped flaky total result
Total 29 1 0 0 30

@DominikGuzei DominikGuzei force-pushed the feat/lw-9887-add-balance-style-to-the-dapp-bridge branch from 9fe2fcb to db655dc Compare March 8, 2024 18:47
@DominikGuzei DominikGuzei force-pushed the feat/lw-9887-add-balance-style-to-the-dapp-bridge branch 2 times, most recently from c618ad5 to 2bf5939 Compare March 11, 2024 19:03
@DominikGuzei DominikGuzei force-pushed the feat/lw-9887-add-balance-style-to-the-dapp-bridge branch 3 times, most recently from 5a3035f to 03544dc Compare March 20, 2024 12:00
@DominikGuzei DominikGuzei force-pushed the feat/lw-9887-add-balance-style-to-the-dapp-bridge branch from 03544dc to d1cb93d Compare March 21, 2024 11:17
Copy link

sonarcloud bot commented Mar 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@DominikGuzei DominikGuzei merged this pull request into feat/lw-9145-tx-summary-refactor Mar 22, 2024
11 of 16 checks passed
@DominikGuzei DominikGuzei deleted the feat/lw-9887-add-balance-style-to-the-dapp-bridge branch March 22, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser Changes to the browser application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants