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

frontend: revamp send confirm dialog / view #2863

Merged

Conversation

shonsirsha
Copy link
Collaborator

@shonsirsha shonsirsha commented Aug 26, 2024

Using View fullscreen instead of WaitDialog and UI improvement in general in order to improve UX (easier to read).

a_ b_

@shonsirsha shonsirsha marked this pull request as draft August 26, 2024 07:06
@shonsirsha shonsirsha force-pushed the frontend-revamp-send-summary branch 3 times, most recently from 0694e58 to 68c0880 Compare August 26, 2024 11:56
@shonsirsha shonsirsha marked this pull request as ready for review August 26, 2024 11:57
}

.confirmationItemWrapper {
align-items: center;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please change so that the left and right numbers are both on the same text base line regardless of the fontsize (currently left value uses bigger font-size).

Suggested change
align-items: center;
align-items: baseline;

}

.totalWrapper {
align-items: center;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
align-items: center;
align-items: baseline;

const canShowTotalFiatValue = (proposedTotal && proposedTotal.conversions) && proposedTotal.conversions[fiatUnit];


return <View fullscreen>
Copy link
Collaborator

Choose a reason for hiding this comment

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

tiny style nit:

Suggested change
return <View fullscreen>
return (
<View fullscreen>

<div className={style.confirmItem}>
<label>{t('button.send')}</label>
<div className={style.confirmationItemWrapper}>
<p className={`${style.valueOriginalLarge}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<p className={`${style.valueOriginalLarge}`}>
<p className={style.valueOriginalLarge}>

{/*Selected UTXOs*/}
{
hasSelectedUTXOs && (
<div className={[style.confirmItem].join(' ')}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<div className={[style.confirmItem].join(' ')}>
<div className={style.confirmItem}>



const FiatValue = ({ baseCurrencyUnit, amount }: TFiatValueProps) => {
return <p className={style.fiatValue}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

style nit:

Suggested change
return <p className={style.fiatValue}>
return (
<p className={style.fiatValue}>

@shonsirsha shonsirsha force-pushed the frontend-revamp-send-summary branch 2 times, most recently from 7a8b168 to d3c6c7c Compare August 26, 2024 15:06
@shonsirsha
Copy link
Collaborator Author

Thanks! PTAL @thisconnect

@shonsirsha
Copy link
Collaborator Author

shonsirsha commented Aug 26, 2024

Screenshots for second commit (design change; font size & text colour).

0da da

Copy link
Collaborator

@thisconnect thisconnect left a comment

Choose a reason for hiding this comment

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

LGTM with design nit


.bitBoxContainer img {
max-width: 220px;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I would just use the device image as is, without this max width, unless it is really necessary for some reason.

.bitBoxContainer img {
padding-right: 16px;
max-width: 190px;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use this graphic in different places, this could make sense in all other places, but lets discuss with @jadzeidan first.



return (
<View fullscreen>
Copy link
Collaborator

Choose a reason for hiding this comment

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

On desktop the content is a bit wide. I think we should limit to width="740px" here (there is already width prop that we use in many places).

I came up with 740px as it looks ok and should work with German banner translation, but ofc this depends how it is translated.

I'll post a screenshot somewhere in dm..

Using `View fullscreen` instead of WaitDialog for bb02
and UI improvement in general in order to improve UX
(easier to read).
@shonsirsha shonsirsha merged commit dc9d389 into BitBoxSwiss:master Aug 28, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants