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

Update swaps network fee tooltip #9614

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Oct 15, 2020

This PR updates the network fee tooltip in the fee card to match the latest designs https://www.figma.com/file/fDtda1cs3MmPXw1MgKswZc/Development---MetaSwaps?node-id=1315%3A1308

The tooltip has been moved from the max fee row to the estimated fee row and its content has been changed.

Demo gif:

fee-card-gas-tooltip-update

@danjm danjm requested a review from a team as a code owner October 15, 2020 09:57
@danjm danjm requested a review from kumavis October 15, 2020 09:57
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@danjm danjm changed the base branch from develop to update-fee-card-storybook October 15, 2020 09:59
@danjm danjm force-pushed the update-swaps-network-fee-tooltip branch from 8dcf433 to 663c222 Compare October 15, 2020 12:05
@metamaskbot
Copy link
Collaborator

Builds ready [663c222]
Page Load Metrics (357 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29443552
domContentLoaded2895853568139
load2905863578139
domInteractive2895843558139

position="top"
contentText={(
<div className="fee-card__info-tooltip-content-container">
<div>{ t('swapGasFeeSummary') }</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these divs be paragraphs instead of divs to maintain semantic HTML?

@@ -22,6 +22,30 @@ export default function FeeCard ({
<div className="fee-card__row-header-text--bold">
{t('swapEstimatedNetworkFee')}
</div>
<div className="fee-card__row-label fee-card__info-tooltip-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

Tooltip exposes two props that would make two of these divs obsolete if we add them to InfoTooltip. containerClassName could be set to fee-card__info-tooltip-content-container and wrapperClassName could be set to fee-card__row-label fee-card__info-tooltip-container and then two extra divs are no longer necessary.

width: 10px;
justify-content: center;

> div {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than relying on tag names I think I would prefer a classname be added to those p tags (if suggestion is taken, div if not)

Base automatically changed from update-fee-card-storybook to develop October 16, 2020 15:59
@danjm danjm force-pushed the update-swaps-network-fee-tooltip branch from 51ad4cc to 69c8b88 Compare October 16, 2020 16:19
@danjm
Copy link
Contributor Author

danjm commented Oct 16, 2020

@brad-decker your comments have been addressed in the latest commit

@metamaskbot
Copy link
Collaborator

Builds ready [69c8b88]
Page Load Metrics (408 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint307941136
domContentLoaded3115944069546
load3125954089646
domInteractive3105944069546

Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

Thank you @danjm looks good!

@danjm danjm merged commit 397e3a2 into develop Oct 19, 2020
@danjm danjm deleted the update-swaps-network-fee-tooltip branch October 19, 2020 08:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants