Skip to content

Commit 8ca78e7

Browse files
DDDDDanicaDDDDDanica
authored
fix(27140): fix styles for Slippage Tolerance Button Group in Transaction Settings Modal (#29246)
This PR aims to fix a broken visual display for Slippage Tolerance Button Group in Transaction Settings Modal. After inspecting the styles for `ButtonGroup`, which is the component applies to this part of UI, has overwritten issues of styles between `button-group__button` and `radio-button`. Hence the solution is to specify css based on variant more clearly and avoid the overwrite problem. <img width="321" alt="Screenshot 2024-12-16 at 18 59 19" src="https://github.com/user-attachments/assets/09a9cce7-a308-4049-a0c6-a001659c301e" /> <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29246?quickstart=1) ## **Related issues** Fixes: #27140 ## **Manual testing steps** 1. Click "Swap" from the home screen. 2. In the swap interface, click the cog icon in the top right to open the transaction settings modal. 3. Observe the slippage tolerance button group—it's visually broken. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <img width="339" alt="1" src="https://github.com/user-attachments/assets/187e1024-0c0c-42fd-9fda-bc72725a7483" /> <!-- [screenshots/recordings] --> ### **After** <img width="766" alt="Screenshot 2024-12-16 at 19 19 04" src="https://github.com/user-attachments/assets/ee4f1eb5-ee47-4ac0-aaee-df65b7346cae" /> <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 18bcad9 commit 8ca78e7

File tree

7 files changed

+143
-100
lines changed

7 files changed

+143
-100
lines changed

‎ui/components/ui/button-group/__snapshots__/button-group-component.test.js.snap

+34-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Jest Snapshot v1, https://goo.gl/fbAQLP
22

3-
exports[`ButtonGroup Component should match snapshot 1`] = `
3+
exports[`ButtonGroup Component should match snapshot with default variant 1`] = `
44
<div>
55
<div
66
class="someClassName"
@@ -28,3 +28,36 @@ exports[`ButtonGroup Component should match snapshot 1`] = `
2828
</div>
2929
</div>
3030
`;
31+
32+
exports[`ButtonGroup Component should match snapshot with radiogroup variant 1`] = `
33+
<div>
34+
<div
35+
class="someClassName radio-button-group"
36+
role="radiogroup"
37+
style="color: red;"
38+
>
39+
<button
40+
aria-checked="false"
41+
class="radio-button-group__button"
42+
data-testid="button-group__button0"
43+
role="radio"
44+
>
45+
<div
46+
class="mockClass"
47+
/>
48+
</button>
49+
<button
50+
aria-checked="true"
51+
class="radio-button-group__button button-group__button--active radio-button-group__button--active"
52+
data-testid="button-group__button1"
53+
role="radio"
54+
/>
55+
<button
56+
aria-checked="false"
57+
class="radio-button-group__button"
58+
data-testid="button-group__button2"
59+
role="radio"
60+
/>
61+
</div>
62+
</div>
63+
`;

‎ui/components/ui/button-group/button-group-component.test.js

+13-3
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,25 @@ describe('ButtonGroup Component', () => {
1616
<button key="a">
1717
<div className="mockClass" />
1818
</button>,
19-
<button key="b"></button>,
20-
<button key="c"></button>,
19+
<button key="b" />,
20+
<button key="c" />,
2121
];
2222

23-
it('should match snapshot', () => {
23+
it('should match snapshot with default variant', () => {
2424
const { container } = renderWithProvider(
2525
<ButtonGroup {...props}>{mockButtons}</ButtonGroup>,
2626
);
2727

2828
expect(container).toMatchSnapshot();
2929
});
30+
31+
it('should match snapshot with radiogroup variant', () => {
32+
const { container } = renderWithProvider(
33+
<ButtonGroup {...props} variant="radiogroup">
34+
{mockButtons}
35+
</ButtonGroup>,
36+
);
37+
38+
expect(container).toMatchSnapshot();
39+
});
3040
});

‎ui/components/ui/button-group/button-group.component.js

+4-3
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,14 @@ export default class ButtonGroup extends PureComponent {
7474
role={variant === 'radiogroup' ? 'radio' : undefined}
7575
aria-checked={index === this.state.activeButtonIndex}
7676
className={classnames(
77-
'button-group__button',
77+
variant === 'radiogroup'
78+
? 'radio-button-group__button'
79+
: 'button-group__button',
7880
child.props.className,
7981
{
80-
'radio-button': variant === 'radiogroup',
8182
'button-group__button--active':
8283
index === this.state.activeButtonIndex,
83-
'radio-button--active':
84+
'radio-button-group__button--active':
8485
variant === 'radiogroup' &&
8586
index === this.state.activeButtonIndex,
8687
},

‎ui/components/ui/button-group/index.scss

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
}
3939

4040
.radio-button-group {
41-
.radio-button {
41+
&__button {
4242
@include design-system.H7;
4343

4444
color: var(--color-text-alternative);

‎ui/pages/swaps/transaction-settings/__snapshots__/transaction-settings.test.js.snap

+4-4
Original file line numberDiff line numberDiff line change
@@ -2,28 +2,28 @@
22

33
exports[`TransactionSettings renders the component with initial props 1`] = `
44
<div
5-
class="button-group transaction-settings__button-group radio-button-group"
5+
class="transaction-settings__button-group radio-button-group"
66
role="radiogroup"
77
>
88
<button
99
aria-checked="false"
10-
class="button-group__button radio-button"
10+
class="radio-button-group__button"
1111
data-testid="button-group__button0"
1212
role="radio"
1313
>
1414
2%
1515
</button>
1616
<button
1717
aria-checked="true"
18-
class="button-group__button radio-button button-group__button--active radio-button--active"
18+
class="radio-button-group__button button-group__button--active radio-button-group__button--active"
1919
data-testid="button-group__button1"
2020
role="radio"
2121
>
2222
3%
2323
</button>
2424
<button
2525
aria-checked="false"
26-
class="button-group__button transaction-settings__button-group-custom-button radio-button"
26+
class="radio-button-group__button transaction-settings__button-group-custom-button"
2727
data-testid="button-group__button2"
2828
role="radio"
2929
>

‎ui/pages/swaps/transaction-settings/index.scss

+3-1
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,16 @@
88
}
99

1010
&__button-group {
11+
display: flex;
12+
flex-direction: row;
13+
1114
& &-custom-button {
1215
cursor: text;
1316
display: flex;
1417
align-items: center;
1518
justify-content: center;
1619
position: relative;
1720
min-width: 72px;
18-
margin-right: 0;
1921
}
2022
}
2123

‎ui/pages/swaps/transaction-settings/transaction-settings.js

+84-87
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
DISPLAY,
1717
SEVERITIES,
1818
FlexDirection,
19+
BlockSize,
1920
} from '../../../helpers/constants/design-system';
2021
import {
2122
Slippage,
@@ -168,97 +169,93 @@ export default function TransactionSettings({
168169
contentText={t('swapSlippageTooltip')}
169170
/>
170171
</Box>
171-
<Box display={DISPLAY.FLEX}>
172-
<ButtonGroup
173-
defaultActiveButtonIndex={
174-
activeButtonIndex === 2 && !customValue
175-
? 1
176-
: activeButtonIndex
177-
}
178-
variant="radiogroup"
179-
newActiveButtonIndex={activeButtonIndex}
172+
<ButtonGroup
173+
defaultActiveButtonIndex={
174+
activeButtonIndex === 2 && !customValue
175+
? 1
176+
: activeButtonIndex
177+
}
178+
variant="radiogroup"
179+
newActiveButtonIndex={activeButtonIndex}
180+
className={classnames('transaction-settings__button-group')}
181+
style={{ width: BlockSize.Half }}
182+
>
183+
<Button
184+
onClick={() => {
185+
setCustomValue('');
186+
setEnteringCustomValue(false);
187+
setActiveButtonIndex(0);
188+
setNewSlippage(Slippage.default);
189+
}}
190+
>
191+
{t('swapSlippagePercent', [Slippage.default])}
192+
</Button>
193+
<Button
194+
onClick={() => {
195+
setCustomValue('');
196+
setEnteringCustomValue(false);
197+
setActiveButtonIndex(1);
198+
setNewSlippage(Slippage.high);
199+
}}
200+
>
201+
{t('swapSlippagePercent', [Slippage.high])}
202+
</Button>
203+
<Button
180204
className={classnames(
181-
'button-group',
182-
'transaction-settings__button-group',
205+
'transaction-settings__button-group-custom-button',
206+
{
207+
'radio-button--danger': isDangerSeverity,
208+
},
183209
)}
210+
onClick={() => {
211+
setActiveButtonIndex(2);
212+
setEnteringCustomValue(true);
213+
}}
184214
>
185-
<Button
186-
onClick={() => {
187-
setCustomValue('');
188-
setEnteringCustomValue(false);
189-
setActiveButtonIndex(0);
190-
setNewSlippage(Slippage.default);
191-
}}
192-
>
193-
{t('swapSlippagePercent', [Slippage.default])}
194-
</Button>
195-
<Button
196-
onClick={() => {
197-
setCustomValue('');
198-
setEnteringCustomValue(false);
199-
setActiveButtonIndex(1);
200-
setNewSlippage(Slippage.high);
201-
}}
202-
>
203-
{t('swapSlippagePercent', [Slippage.high])}
204-
</Button>
205-
<Button
206-
className={classnames(
207-
'transaction-settings__button-group-custom-button',
208-
{
209-
'radio-button--danger': isDangerSeverity,
210-
},
211-
)}
212-
onClick={() => {
213-
setActiveButtonIndex(2);
214-
setEnteringCustomValue(true);
215-
}}
216-
>
217-
{enteringCustomValue ? (
218-
<div
219-
className={classnames(
220-
'transaction-settings__custom-input',
221-
{
222-
'transaction-settings__custom-input--danger':
223-
isDangerSeverity,
224-
},
225-
)}
226-
>
227-
<input
228-
data-testid="transaction-settings-custom-slippage"
229-
onChange={(event) => {
230-
const { value } = event.target;
231-
const isValueNumeric = !isNaN(Number(value));
232-
if (isValueNumeric) {
233-
setCustomValue(value);
234-
setNewSlippage(Number(value));
235-
}
236-
}}
237-
type="text"
238-
maxLength="4"
239-
ref={setInputRef}
240-
onBlur={() => {
215+
{enteringCustomValue ? (
216+
<div
217+
className={classnames(
218+
'transaction-settings__custom-input',
219+
{
220+
'transaction-settings__custom-input--danger':
221+
isDangerSeverity,
222+
},
223+
)}
224+
>
225+
<input
226+
data-testid="transaction-settings-custom-slippage"
227+
onChange={(event) => {
228+
const { value } = event.target;
229+
const isValueNumeric = !isNaN(Number(value));
230+
if (isValueNumeric) {
231+
setCustomValue(value);
232+
setNewSlippage(Number(value));
233+
}
234+
}}
235+
type="text"
236+
maxLength="4"
237+
ref={setInputRef}
238+
onBlur={() => {
239+
setEnteringCustomValue(false);
240+
}}
241+
onKeyDown={(event) => {
242+
if (event.key === 'Enter') {
241243
setEnteringCustomValue(false);
242-
}}
243-
onKeyDown={(event) => {
244-
if (event.key === 'Enter') {
245-
setEnteringCustomValue(false);
246-
}
247-
}}
248-
value={customValue || ''}
249-
/>
250-
</div>
251-
) : (
252-
customValueText
253-
)}
254-
{(customValue || enteringCustomValue) && (
255-
<div className="transaction-settings__percentage-suffix">
256-
%
257-
</div>
258-
)}
259-
</Button>
260-
</ButtonGroup>
261-
</Box>
244+
}
245+
}}
246+
value={customValue || ''}
247+
/>
248+
</div>
249+
) : (
250+
customValueText
251+
)}
252+
{(customValue || enteringCustomValue) && (
253+
<div className="transaction-settings__percentage-suffix">
254+
%
255+
</div>
256+
)}
257+
</Button>
258+
</ButtonGroup>
262259
</>
263260
)}
264261
</>

0 commit comments

Comments
 (0)