feat: [UI - Swap] Create row radiobutton component with custom field #14887

caybro · 2024-05-23T08:24:03Z

What does the PR do

creates 2 components with respective SB pages:
- StatusButtonRow: generic row of buttons working with a model with customizable text/value roles
- SlippageSelector: composition of the above and a custom button/input for user defined value (will be used for SwapModal)
Create necessary qml tests to cover the component logic

Fixes #14784

Affected areas

StatusButtonRow, SlippageSelector

Screenshot of functionality (including design for comparison)

I've checked the design and this PR matches it

StatusButtonRow:

SlippageSelector:

Zaznam.obrazovky.z.2024-05-28.00-00-33.webm

status-im-auto · 2024-05-23T08:31:08Z

Jenkins Builds

Click to see older builds (36)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`f2f1b1f`	#1	2024-05-23 08:31:08	~6 min	`macos/aarch64`	🍎`dmg`
✔️	`f2f1b1f`	#1	2024-05-23 08:34:52	~10 min	`tests/nim`	📄`log`
✔️	`f2f1b1f`	#1	2024-05-23 08:36:54	~12 min	`macos/x86_64`	🍎`dmg`
✔️	`f2f1b1f`	#1	2024-05-23 08:41:48	~17 min	`tests/ui`	📄`log`
✔️	`f2f1b1f`	#1	2024-05-23 08:46:50	~22 min	`linux/x86_64`	📦`tgz`
✔️	`f2f1b1f`	#1	2024-05-23 08:48:50	~24 min	`windows/x86_64`	💿`exe`

✔️	`d424572`	#2	2024-05-27 08:45:06	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`d424572`	#2	2024-05-27 08:48:25	~7 min	`macos/x86_64`	🍎`dmg`

✔️	`08895e9`	#3	2024-05-27 08:55:25	~5 min	`tests/nim`	📄`log`
✔️	`08895e9`	#3	2024-05-27 08:55:46	~6 min	`macos/aarch64`	🍎`dmg`
✔️	`08895e9`	#3	2024-05-27 08:57:53	~8 min	`macos/x86_64`	🍎`dmg`
❌	`08895e9`	#3	2024-05-27 09:05:36	~15 min	`tests/ui`	📄`log`
✔️	`08895e9`	#3	2024-05-27 09:07:29	~17 min	`linux/x86_64`	📦`tgz`
✔️	`08895e9`	#3	2024-05-27 09:14:18	~24 min	`windows/x86_64`	💿`exe`

✔️	`796bc92`	#4	2024-05-27 13:28:59	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`796bc92`	#4	2024-05-27 13:32:32	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`796bc92`	#4	2024-05-27 13:34:18	~9 min	`tests/nim`	📄`log`
❌	`796bc92`	#4	2024-05-27 13:40:59	~16 min	`tests/ui`	📄`log`
✔️	`796bc92`	#4	2024-05-27 13:44:05	~19 min	`linux/x86_64`	📦`tgz`
✔️	`796bc92`	#4	2024-05-27 13:48:30	~24 min	`windows/x86_64`	💿`exe`

✔️	`458cb53`	#5	2024-05-27 13:58:43	~3 min	`macos/aarch64`	🍎`dmg`
✔️	`458cb53`	#5	2024-05-27 14:02:03	~7 min	`macos/x86_64`	🍎`dmg`
✔️	`458cb53`	#5	2024-05-27 14:04:45	~10 min	`tests/nim`	📄`log`
❌	`458cb53`	#5	2024-05-27 14:10:14	~15 min	`tests/ui`	📄`log`
✔️	`458cb53`	#5	2024-05-27 14:14:55	~20 min	`linux/x86_64`	📦`tgz`
✔️	`458cb53`	#5	2024-05-27 14:19:02	~24 min	`windows/x86_64`	💿`exe`

✔️	`1800226`	#6	2024-05-27 17:43:00	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`1800226`	#6	2024-05-27 17:46:32	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`1800226`	#6	2024-05-27 17:48:21	~9 min	`tests/nim`	📄`log`

✔️	`794cdf0`	#7	2024-05-27 17:57:36	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`794cdf0`	#7	2024-05-27 18:00:48	~7 min	`macos/x86_64`	🍎`dmg`
✔️	`794cdf0`	#7	2024-05-27 18:02:59	~9 min	`tests/nim`	📄`log`
❌	`794cdf0`	#7	2024-05-27 18:05:17	~11 min	`tests/ui`	📄`log`
✔️	`794cdf0`	#7	2024-05-27 18:14:44	~21 min	`linux/x86_64`	📦`tgz`
✔️	`794cdf0`	#7	2024-05-27 18:17:32	~23 min	`windows/x86_64`	💿`exe`
✔️	`794cdf0`	#8	2024-05-27 19:02:42	~15 min	`tests/ui`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`131b597`	#8	2024-05-27 21:07:08	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`131b597`	#8	2024-05-27 21:12:34	~9 min	`macos/x86_64`	🍎`dmg`
✔️	`131b597`	#8	2024-05-27 21:12:57	~9 min	`tests/nim`	📄`log`
✔️	`131b597`	#9	2024-05-27 21:18:52	~15 min	`tests/ui`	📄`log`
✔️	`131b597`	#8	2024-05-27 21:22:52	~19 min	`linux/x86_64`	📦`tgz`
✔️	`131b597`	#8	2024-05-27 21:26:23	~23 min	`windows/x86_64`	💿`exe`

✔️	`914f90e`	#10	2024-05-27 22:06:37	~6 min	`tests/nim`	📄`log`
✔️	`914f90e`	#10	2024-05-27 22:06:52	~6 min	`macos/aarch64`	🍎`dmg`
✔️	`914f90e`	#11	2024-05-27 22:12:33	~12 min	`tests/ui`	📄`log`
✔️	`914f90e`	#10	2024-05-27 22:13:07	~12 min	`macos/x86_64`	🍎`dmg`
✔️	`914f90e`	#10	2024-05-27 22:20:02	~19 min	`linux/x86_64`	📦`tgz`
✔️	`914f90e`	#10	2024-05-27 22:23:38	~23 min	`windows/x86_64`	💿`exe`

micieslak

Looks nice in general, however I have an idea for a bit different structure of code:

The idea is to make the StatusButtonRow possibly generic row of buttons, with API inspired by ComboBox:

StatusButtonRow {

   // writable properties:
   var model
   var defaultValue
   string textRole
   string valueRole

   // readonly properties:
   int currentIndex
   int currentText
   var currentValue
}

Then StatusButtonRow gives the user full freedom what is value, what is text and how it's composed. Currently we are limited to numbers with customizable suffix.

Then, using this StatusButtonRow via composition as a building block we could build the specific component we need, e.g. SlippageSelector. It would contain buttons row, additional input field and the whole additional logic needed here. However it would be simpler than current impl, because button row logic would be externalized, and we don't need loaders for and logic for controlling custom component.

The public api of the SlippageSelector could be very simple, even just

SlippageSelector {
   // readonly props:
   bool valid
   double value
}

It will be also easier to cover some specific needs of this case - e.g. now 200 % is presented as valid input and there is no way via api of that component to mark it as invalid.

I think that this approach is more resistant to components overparametrization. When new case base of button rows is needed, generalized and simplified StatusButtonRow can be used as a building block without modifications and also SlippageSelector stays intact.

Playing with the component, I noticed some small undesired behavior regarding red border around input field:

Screencast.from.24.05.2024.14.14.09.webm

Please shared wdyt regarding structure. Thanks!

caybro · 2024-05-24T13:30:20Z

StatusButtonRow {

// writable properties:
var model
var defaultValue
string textRole
string valueRole

// readonly properties:
int currentIndex
int currentText
var currentValue
}

Pretty much agree overall, I can definitely publish the readonly properties so that they can be consumed from outside; however I think separating textRole/valueRole is a bit overkill, we will likely want to always display "values" here (optionally formatted according to locale or that custom symbol but that's implementation detail)

It will be also easier to cover some specific needs of this case - e.g. now 200 % is presented as valid input and there is no way via api of that component to mark it as invalid.

Yeah for this specific usecase I don't have a clear idea/answer yet; I'd have to somehow make the customInput's validator public/customizable.

Playing with the component, I noticed some small undesired behavior regarding red border around input field:

Most likely rounding errors, can't see anything like that on a highres screen :/

caybro · 2024-05-24T13:32:02Z

// readonly properties:
var currentValue

Oh and this has to be writable imho (as it is currently), you still might want to preselect a custom (previously saved) value that's not part of the model

caybro · 2024-05-24T13:33:44Z

@micieslak the red/validation border rect:

Zaznam.obrazovky.z.2024-05-24.15-32-57.webm

micieslak · 2024-05-24T13:40:26Z

@micieslak the red/validation border rect:

Zaznam.obrazovky.z.2024-05-24.15-32-57.webm

Screencast.from.24.05.2024.15.38.50.webm

First click deselect input, the second - not

micieslak · 2024-05-24T13:47:27Z

Pretty much agree overall, I can definitely publish the readonly properties so that they can be consumed from outside; however I think separating textRole/valueRole is a bit overkill, we will likely want to always display "values" here (optionally formatted according to locale or that custom symbol but that's implementation detail)

textRole/valueRole is a bit more then needed in that case, but I can't see how other dissect values from text representation properly. However it's quite easy to do, in delegate we can use model[textRole] for display and model[valueRole] for setting current value.

Yeah for this specific usecase I don't have a clear idea/answer yet; I'd have to somehow make the customInput's validator public/customizable.

For that reason I would opt for stop trying to parametrize all things here and do just custom component at the end of the chain implementing the specific logic of that case.

Playing with the component, I noticed some small undesired behavior regarding red border around input field:

is the second deselection working as intended?

caybro · 2024-05-24T13:55:54Z

Playing with the component, I noticed some small undesired behavior regarding red border around input field:

is the second deselection working as intended?

yes

micieslak · 2024-05-24T14:02:52Z

Screencast.from.24.05.2024.16.00.37.webm

Probably in that case, the component should switch into "custom mode"

caybro · 2024-05-24T14:04:45Z

Screencast.from.24.05.2024.16.00.37.webm
Probably in that case, the component should switch into "custom mode"

Yeah right, setting the currentValue during runtime will require some more code :)

- Create row radiobutton component like the one defined in design - It shall contain custom set of buttons - It shall contain a Custom button that will be converted to an input field - Add the new component into a new storybook page - Create necessary qml tests to cover the component logic Fixes #14784

…mplified, fixed issue with setting values from outside

- also split the SB pages

do not overwrite it and display the Custom button when we click one of the predefined buttons

micieslak

Cool! Just some minor comments.

storybook/pages/SlippageSelectorPage.qml

storybook/qmlTests/tests/tst_SlippageSelector.qml

storybook/pages/StatusButtonRowPage.qml

Khushboo-dev-cpp

LGTM! Awesome work.

caybro linked an issue May 23, 2024 that may be closed by this pull request

[UI - Swap] Create row radiobutton component with custom field #14784

Closed

caybro requested review from micieslak, Seitseman, alexjba and Khushboo-dev-cpp May 23, 2024 08:24

caybro requested a review from dlipicar May 24, 2024 12:09

micieslak reviewed May 24, 2024

View reviewed changes

micieslak force-pushed the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch from d424572 to 08895e9 Compare May 27, 2024 08:49

caybro marked this pull request as draft May 27, 2024 09:55

caybro force-pushed the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch from 08895e9 to 796bc92 Compare May 27, 2024 13:24

caybro marked this pull request as ready for review May 27, 2024 17:38

caybro force-pushed the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch from 1800226 to 794cdf0 Compare May 27, 2024 17:53

caybro and others added 5 commits May 27, 2024 23:00

StatusRowButton and SlippageSelector as a separate components, api si…

a047a64

…mplified, fixed issue with setting values from outside

fix(StatusButtonRow): move to StatusQ

61a76f5

- also split the SB pages

fix(SlippageSelector): remember previous custom value

a9da97c

do not overwrite it and display the Custom button when we click one of the predefined buttons

chore(SlippageSelector): rename and move tests

4cae83a

caybro force-pushed the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch from 794cdf0 to 131b597 Compare May 27, 2024 21:02

micieslak approved these changes May 27, 2024

View reviewed changes

caybro force-pushed the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch from 131b597 to 8823862 Compare May 27, 2024 21:56

fix(SlippageSelector): fixup custom input focus and tests

914f90e

caybro force-pushed the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch from 8823862 to 914f90e Compare May 27, 2024 22:00

Khushboo-dev-cpp approved these changes May 28, 2024

View reviewed changes

caybro merged commit 02a17b6 into master May 28, 2024
8 checks passed

caybro deleted the 14784-ui-swap-create-row-radiobutton-component-with-custom-field branch May 28, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [UI - Swap] Create row radiobutton component with custom field #14887

feat: [UI - Swap] Create row radiobutton component with custom field #14887

caybro commented May 23, 2024 •

edited

Loading

status-im-auto commented May 23, 2024 •

edited

Loading

micieslak left a comment

caybro commented May 24, 2024

caybro commented May 24, 2024 •

edited

Loading

caybro commented May 24, 2024

micieslak commented May 24, 2024

micieslak commented May 24, 2024

caybro commented May 24, 2024

micieslak commented May 24, 2024

caybro commented May 24, 2024

micieslak left a comment

Khushboo-dev-cpp left a comment

feat: [UI - Swap] Create row radiobutton component with custom field #14887

feat: [UI - Swap] Create row radiobutton component with custom field #14887

Conversation

caybro commented May 23, 2024 • edited Loading

What does the PR do

Affected areas

Screenshot of functionality (including design for comparison)

status-im-auto commented May 23, 2024 • edited Loading

Jenkins Builds

micieslak left a comment

Choose a reason for hiding this comment

caybro commented May 24, 2024

caybro commented May 24, 2024 • edited Loading

caybro commented May 24, 2024

micieslak commented May 24, 2024

micieslak commented May 24, 2024

caybro commented May 24, 2024

micieslak commented May 24, 2024

caybro commented May 24, 2024

micieslak left a comment

Choose a reason for hiding this comment

Khushboo-dev-cpp left a comment

Choose a reason for hiding this comment

caybro commented May 23, 2024 •

edited

Loading

status-im-auto commented May 23, 2024 •

edited

Loading

caybro commented May 24, 2024 •

edited

Loading