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.

Sign up for GitHub

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

Jump to bottom

[LW 10157] Show collateral deposits fee outside tx summary #1070

Merged
merged 13 commits into from
Apr 25, 2024
Merged

[LW 10157] Show collateral deposits fee outside tx summary #1070

merged 13 commits into from
Apr 25, 2024

Conversation

lucas-barros
Copy link

@lucas-barros lucas-barros commented Apr 15, 2024 •
edited
Loading

Checklist

Proposed solution

Features

  • New “Additional information” title, heading a separated section that comprises fees,deposits and collateral
  • New dividing line between Tx Summary and Additional information
  • Added tooltip for “transaction summary”

Fixes

  • Fallback image for asset was expecting a string but was passed a encoded image
  • Font color and weight adjustments

Testing

Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met

Screenshots

image image

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 15, 2024 •
edited
Loading

Allure report

allure-report-publisher generated test report!

smokeTests: ✅ test report for 498939e4

passed failed skipped flaky total result
Total 27 0 0 0 27 ✅

@lucas-barros lucas-barros marked this pull request as ready for review April 15, 2024 19:21
@lucas-barros lucas-barros requested a review from a team as a code owner April 15, 2024 19:21
pczeglik-iohk
pczeglik-iohk reviewed Apr 16, 2024
View reviewed changes
packages/core/src/ui/components/DappAddressSections/DappAddressSections.tsx Outdated
<SummaryExpander
title={t('core.dappTransaction.fromAddress')}
disabled={!isFromAddressesEnabled}
testId="dapp-transaction-from-section-expander"
>
<div className={styles.summaryContent}>
<div className={styles.fromAddress}>
Copy link
Contributor

Choose a reason for hiding this comment

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

SonarCloud complains because of the code duplication in this file.

Can we maybe break this looong return into a few sub components?

I see there is a pattern. We have dapp-transaction-from-row and dapp-transaction-to-row which looks the same, the only difference is the value we put.

I think if we break this long return into smaller components we can gain better maintenance and code reusability. We can start from local components. We may not need to put them into separate files.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

There are more differences than the value, also the class applied to some components and "minus" sign for the "from" section.
I created a new component and cover these cases here: 2467580.

pczeglik-iohk
pczeglik-iohk reviewed Apr 16, 2024
View reviewed changes
packages/ui/src/design-system/dapp-transaction-summary/dapp-transaction-summary.component.tsx Outdated Show resolved Hide resolved
packages/ui/src/design-system/dapp-transaction-summary/dapp-transaction-assets.component.tsx Outdated
@@ -65,7 +65,7 @@ export const TransactionAssets = ({
<Grid {...props} columns="$fitContent">
<Cell>
<UserProfile
fallback={setThemeFallbackImagine}
fallbackLogo={setThemeFallbackImagine}
Copy link
Contributor

Choose a reason for hiding this comment

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

another place where we have inconsistency. We have fallbackLogo and we are setting Image. Good to have it aligned. By the way, what this Imagine is? Can we change it to setThemFallbackImage, please :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is valid.
I would say maybe this would require its own ticket for refactoring. The component is also called UserProfile but it's used to set the images of assets in dapp transaction. Initially it wasn't created for this purpose. We could open a ticket to address this and improve the component in all the app.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed here: bf29a29

VanessaPC
VanessaPC reviewed Apr 16, 2024
View reviewed changes
packages/core/src/ui/components/DappTransaction/DappTransaction.module.scss Outdated
margin-bottom: size_unit(2.5);
}

.divider {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a divider that is general for the app that we could import? if not, is it worth making this one?

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a component elsewhere. I think is a good idea for a new task, as to not increase the scope here.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this something you are looking for? https://github.com/input-output-hk/lace/blob/chore/deps-update-cardano-js-sdk/packages/ui/src/design-system/divider/divider.component.tsx#L12

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thank you. I looked in storybook, but I didn't find it since there is no story for this component.
Fixed here: e7f76d6

VanessaPC
VanessaPC reviewed Apr 16, 2024
View reviewed changes
Copy link
Contributor

@VanessaPC VanessaPC left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work! I left a small couple of comments but it looks nice! Do you think there is better way to structure the file of DappAddressContainer? and break it into smaller components? or not have code duplication?

@lucas-barros
Copy link
Author

Great work! I left a small couple of comments but it looks nice! Do you think there is better way to structure the file of DappAddressContainer? and break it into smaller components? or not have code duplication?

Thank you. Do you mean DappAddressSections? If yes, I updated here: 2467580

pczeglik-iohk
pczeglik-iohk approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@pczeglik-iohk pczeglik-iohk left a comment

Choose a reason for hiding this comment

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

Great work @lucas-barros

DominikGuzei
DominikGuzei reviewed Apr 17, 2024
View reviewed changes
packages/core/src/ui/lib/translations/en.json Outdated
@@ -422,6 +422,8 @@
"address": "Address",
"origin": "Origin",
"transactionSummary": "Transaction Summary",
"transactionSummaryTooltip": "This summary includes all assets entering or leaving your wallet as part of this transaction, including fees, deposits.",
"additionalInformation": "Additional information",
Copy link
Member

Choose a reason for hiding this comment

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

I think the additionalInformation should be moved to the browser translations file to avoid the long load times (we had this issue for a while that for a few seconds you see the translation path until the core translations are loaded)

DominikGuzei
DominikGuzei approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

Great work 👏

VanessaPC
VanessaPC approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@VanessaPC VanessaPC left a comment

Choose a reason for hiding this comment

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

LGTM!

@lucas-barros lucas-barros force-pushed the feat/lw-10157-show-collateral-deposits-fee-outside-tx-summary branch 4 times, most recently from 97f5c82 to b07b759 Compare April 19, 2024 12:34
@lucas-barros lucas-barros force-pushed the feat/lw-10157-show-collateral-deposits-fee-outside-tx-summary branch from b07b759 to 7a23b13 Compare April 23, 2024 14:59
@lucas-barros lucas-barros force-pushed the feat/lw-10157-show-collateral-deposits-fee-outside-tx-summary branch from ab8e314 to d220bfe Compare April 24, 2024 14:58
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@lucas-barros lucas-barros merged commit 7cae9ba into main Apr 25, 2024
15 checks passed
@lucas-barros lucas-barros deleted the feat/lw-10157-show-collateral-deposits-fee-outside-tx-summary branch April 25, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pczeglik-iohk pczeglik-iohk pczeglik-iohk approved these changes

@VanessaPC VanessaPC VanessaPC approved these changes

@DominikGuzei DominikGuzei DominikGuzei approved these changes

@renanvalentin renanvalentin Awaiting requested review from renanvalentin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lucas-barros @DominikGuzei @VanessaPC @pczeglik-iohk