Order input component style and code polishing #117

abdulhaseeb13mar · 2023-09-14T17:03:19Z

fixes #141
fixes #138

I have left the styles of the components that were not present in figma, we need to decide a relevant design for that.

netlify · 2023-09-14T17:03:25Z

✅ Deploy Preview for chipper-sunburst-578cfe ready!

Name	Link
🔨 Latest commit	`1ea517b`
🔍 Latest deploy log	https://app.netlify.com/sites/chipper-sunburst-578cfe/deploys/652fa86778c1740008f50f51
😎 Deploy Preview	https://deploy-preview-117--chipper-sunburst-578cfe.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

EvgeniiaVak

The percentage shortcuts do not work at all now (before the PR they are working (although with bugs, we should fix them separately) and setting the position size).

Maybe hiding the shortcuts for the MVP if they cause a lot of trouble? We don't have the UI defined for them either.

EvgeniiaVak · 2023-09-15T08:22:11Z

src/app/components/OrderInput.tsx

+  const [customPercentage, setCustomPercentage] = useState(0);
+  const [isFirstInteraction, setIsFirstInteraction] = useState(true);


Let's try to not use useState since we already have redux, and use only redux for state management.

What is the idea for fist interaction? Why is it something different from any other interaction?

Let's try to not use useState since we already have redux, and use only redux for state management.

What is the idea for fist interaction? Why is it something different from any other interaction?

both these states are the local states of the component. i don't think we should move these state in redux as it is not encouraged Read Here

Reasons why these two states:
customPercentage was needed to handle the current percentage selection through which i am changing the color of the active percentage box and also handling the input state of the component.

isFirstInteraction: whenever we load the dexter app it starts showing error on the inputs even though user has not even entered anything yet. therefore, i am keeping this state as a memory for the input field so it will not show error if user has not yet entered anything in that specific input field.

About isFirstInteraction , I think it's more user-friendly to show the errors if any exist immediately - quickly explains why the submit button is disabled and directs attention to editing them. Maybe instead we can use different defaults, so they would be valid from the start?

OK for useState in this case, but (for other cases) I would argue even if the state is local to a component, but requires some network processing or transformation logic - it's better moving them to redux slices, the code will be much more readable easier to test and debug.

maybe we need to come up with some other UX for the inputs? because clearly they don't look good the moment you load Dexter and you are welcomed with 2 big red outlined input fields. We should wait for the user to interact with these input fields then we can show them the errors. (That's my thought behind using isFirstInteraction )

regarding customPercentage i was looking for some redux state for current selected percentage but i didn't find any,
we are only using this const size = useAppSelector((state) => state.orderInput.size); state but it returns the calculated size. And since i didn't find any need to move it to redux because we were not using the current selected percentage anywhere else so i made it a local component state. i can move it to redux if you think we might be needing this state somewhere else.

We should wait for the user to interact with these input fields then we can show them the errors. (That's my thought behind using isFirstInteraction )

Yes, I understand why you introduce the isFirstInteraction, the alternative suggestion is to leave the validation always active but substitute the defaults with something more reasonable than 0s, so that there wouldn't be an error message right away.

maybe we need to come up with some other UX for the inputs?

a related comment in discord - https://discord.com/channels/1125689933735145503/1146445162508197888/1152192758942023721

About customPercentage, if we have it, we would need to react to size input here as well. We can theoretically use only size in the state, and when the user types in the size itself, the custom percentage would calculate out of it and get updated also, it would be nice to have, but I don't think it's important to have in the MVP and it will take time to develop and test. I would suggest leaving custom percentage being wiped out (as it is now in the main branch) when user either inputs size or clicks percentage shortcuts, because it's simpler.

i will put a placeholder "0" instead of real value, that way we might be able to get rid of isFirstInteraction

Although we are using custom percentage it's just white so we didn't really notice, i made it black in screenshot

we can hide it for now. but this customPercentage state is also being used by the percentage buttons. so this state would still be there.

It is possible to go without customPercentage state if when user licks on shortcuts or enters size (aka amount), the custom percentage input clears, that would simplify the code.

It's OK to have the customPercentage, but if we have it we need to synchronize the size input also - meaning when the user enters the size (amount) itself the custom percentage should also recalculate.

I would even go so far as to remove the shortcuts for the MVP at all - they are buggy and pollute already quite noisy input ui, and we can introduce them again later, more tested and together with the redesign?

abdulhaseeb13mar · 2023-09-15T10:59:57Z

The percentage shortcuts do not work at all now (before the PR they are working (although with bugs, we should fix them separately) and setting the position size).

Maybe hiding the shortcuts for the MVP if they cause a lot of trouble? We don't have the UI defined for them either.

@EvgeniiaVak i tried clicking percentage boxes by connecting wallet and they seem to be working (in the sense that the values are changing in position size input) but i don't know if the position size value is being calculated correctly. There seems to be some error coming from the SDK about insufficient liquidity.

EvgeniiaVak · 2023-09-18T06:48:33Z

src/app/components/OrderInput.tsx

+  const [customPercentage, setCustomPercentage] = useState(0);
+  const [isFirstInteraction, setIsFirstInteraction] = useState(true);


It is possible to go without customPercentage state if when user licks on shortcuts or enters size (aka amount), the custom percentage input clears, that would simplify the code.

It's OK to have the customPercentage, but if we have it we need to synchronize the size input also - meaning when the user enters the size (amount) itself the custom percentage should also recalculate.

I would even go so far as to remove the shortcuts for the MVP at all - they are buggy and pollute already quite noisy input ui, and we can introduce them again later, more tested and together with the redesign?

abdulhaseeb13mar · 2023-09-23T12:16:57Z

@EvgeniiaVak i tried to revamp the functionality according to my thoughts mixing up with Vlad's design and the older designs on figma. i would suggest to merge it and get feedback from the TG community before working on it further. please review it and let me know about futher changes or tweaks if any.

EvgeniiaVak · 2023-09-24T12:32:19Z

@abdulhaseeb13mar I briefly tried it out on https://deploy-preview-117--chipper-sunburst-578cfe.netlify.app/ and noticed 2 things :

there shouldn't be a toggle about preventing immediate execution on market orders
we need to be able to switch the token in which we specify the amount

I think it's better to remove the balance word from the order input at all, and display it in the pair selector component (like in Vlad's figma).

Sorry, I don't have much time this weekend for a more detailed code review, if nobody else reviews by then, I will check tomorrow.

abdulhaseeb13mar · 2023-09-24T13:55:06Z

@EvgeniiaVak I thought about that switching approach, Like Uniswap or pancake swap and some major swaps, they use a switching approach because they do not give buy or sell tabs.
If we are going with the BUY/SELL approach. Then user can switch the tabs.

i.e ADEX2/XRD pair

Buy ADEX2, switch to the Buy tab. Write ADEX2 amount to buy.
Sell ADEX2, switch to the Sell tab. Write ADEX2 amount to sell.
Buy XRD, (means we are selling ADEX2), switch to the Sell tab. Write ADEX2 amount to sell.
Sell XRD, (means we are buying ADEX2), switch to the Buy tab. Write ADEX2 amount to buy.

That's why the BUY/SELL approach does not need to switch the token.

abdulhaseeb13mar · 2023-09-24T16:31:27Z

my motivation for showing balance above the input field came from pancake swap, and many other dapps that i have seen.
tho now i have just commented out the balance from there for now. we can discuss this further in TG group maybe.

EvgeniiaVak · 2023-09-25T11:05:59Z

If we are aiming to this design eventually, having balances near the input fields will probably make it too noisy.

What happened to the % shortcuts? I've seen that @SmashingBumpkin had fixed them here in this PR, so they probably work correctly now? Did you decide to hide them anyways?

EvgeniiaVak · 2023-09-25T12:18:32Z

Started the discussion on tg - https://t.me/c/1988854086/1/383

abdulhaseeb13mar · 2023-09-25T12:52:57Z

What happened to the % shortcuts? I've seen that @SmashingBumpkin had fixed them here in this PR, so they probably work correctly now? Did you decide to hide them anyways?

they were still not working correctly, so i decided to hide them for now, as it was consuming too much time. decided to work on it after the MVP launch or at the end of all tasks.

fliebenberg · 2023-09-25T13:26:10Z

my motivation for showing balance above the input field came from pancake swap, and many other dapps that i have seen. tho now i have just commented out the balance from there for now. we can discuss this further in TG group maybe.

I think for our purposes it is OK to show the balances at the top as in the design. Because we work in the context of a selected pair, we can show the balance for both tokens. For Pancake Swap (and other AMMs) the user can normally select from one of many tokens, and therefore you will only show him the balance of the token he currently has selected (as you can't show him balances for all tokens)
.

…ter-frontend into order-input

abdulhaseeb13mar · 2023-10-13T19:54:36Z

src/app/components/order_input/MarketOrderInput.tsx

+  selectTargetToken,
+  validateSlippageInput,
+} from "redux/orderInputSlice";
+import { useEffect } from "react";


forgot to move the library import to the top separated by an empty line from the local imports

fliebenberg · 2023-10-14T09:01:40Z

Have been playing around with this and have a few comments for LIMIT order pages:

I think it might be useful to still have an "Amount" heading above the section where you have to put in the amount for the trade and a "Price" heading above the section where you have the put in the price. When you look at the screen with no values filled in, it is not clear where you actually have to type stuff. E.g. when you get the scren initially, there is just nothing where the 12000 in the picture is reflected and it is not clear at all that you need to type something in the big black area next to FOTON. It might be clearer if the part of the box with FOTON is slightly seperate from the part of the box where you actually have to type in the amount.
Why is the price input field in green/yellow and the amount input field in white?
For the price input field, it shows a zero currently. When I enter a number, the 0 stays at the front and I have to manually delete it. The 0 should just be replaced with the number I am typing.
I cannot enter a . for a decimal in the price input field. However I can enter a , for decimal. I can then delete the comma and replace it with a . and it works. To be consistent with the rest of the app, the field should accept a . as decimal separator.

fliebenberg · 2023-10-14T09:01:55Z

For the MARKET order page, I think we should change SLIPPAGE to SLIPPAGE LIMIT. As it might look to user that that is the actual slippage they will experience, rather than just a limit for slippage.

EvgeniiaVak · 2023-10-14T15:28:29Z

@fliebenberg

I think it might be useful to still have an "Amount" heading above the section ...

I don't think it's confusing without "Amount" label, and we don't have much space in this design for the word amount to be added. I suggest we leave it like this and after merge to our test deployment we can ask the community if that's confusing or not.

Personally, I think this one is the best (if we have to have the buy/sell tabs):

But I also think we don't need the buy/sell tabs and this is the best version (consistent with the market tab):

And for the MVP I suggest we move forward with the current Vlad's design, which we all agreed upon in telegram:

Why is the price input field in green/yellow and the amount input field in white?

🤷 it's in the figma, I think it's ok, maybe the idea is to make the visual difference between price and amounts more apperent

For the price input field, it shows a zero currently. When I enter a number, the 0 stays at the front and I have to manually delete it. The 0 should just be replaced with the number I am typing.

👍 fixing...

I cannot enter a . for a decimal in the price input field. However I can enter a , for decimal. I can then delete the comma and replace it with a . and it works. To be consistent with the rest of the app, the field should accept a . as decimal separator.

👍 fixing...

EvgeniiaVak · 2023-10-14T16:41:17Z

@fliebenberg update on separators - turns out browsers decide on whether to use , or . as separators based on your system settings. Here is how this setting looks like in macOS:

Changing this setting and reloading the browser made DeXter page using the . everywhere. I bet there are similar settings on windows and linux. I think it's a good practice to maintain this default and let the user decide how they want to see their separators. So I vote for reverting back to default if we have some strictly specified . somewhere instead of fixing these inputs.

nguvictor · 2023-10-14T17:25:51Z

There is a bug when switching the pairs when there are decimals.

nguvictor · 2023-10-14T19:11:00Z

It looks like there is an error when calculating cost when using decimals:
Here is an example: I'm expecting to buy 1 ADEX1 for 0.00001, the cost should be 0.00001, but the calculation for price is 10000

fliebenberg · 2023-10-15T14:09:09Z

@fliebenberg update on separators - turns out browsers decide on whether to use , or . as separators based on your system settings. Here is how this setting looks like in macOS:
Changing this setting and reloading the browser made DeXter page using the `.` everywhere. I bet there are similar settings on windows and linux. I think it's a good practice to maintain this default and let the user decide how they want to see their separators. So I vote for reverting back to default if we have some strictly specified `.` somewhere instead of fixing these inputs.

It is fine to revert back to the defaults for the input fields, but then we also need to use the defaults when displaying the amounts. We can add a simple function to read the default separators for the user and then store it and use it in the displayAmount function.

EvgeniiaVak · 2023-10-16T07:46:47Z

It is fine to revert back to the defaults for the input fields, but then we also need to use the defaults when displaying the amounts. We can add a simple function to read the default separators for the user and then store it and use it in the displayAmount function.

@fliebenberg By displaying amounts you mean in the order input component, then it's done in your computer settings. I did and for me the amounts there are with .. People with , as their preferred separator will have ,.

If you mean in the utils.displayAmount, yes we need to use the user's separator there, but as far as I know there is no simple way to do read this setting from the webpage. Maybe we can use JavaScript's toString inside the function somehow. Anyways it requires some time and fiddling with, I suggest we do it in a separate PR. #76

fliebenberg · 2023-10-16T08:29:41Z

It is fine to revert back to the defaults for the input fields, but then we also need to use the defaults when displaying the amounts. We can add a simple function to read the default separators for the user and then store it and use it in the displayAmount function.

@fliebenberg By displaying amounts you mean in the order input component, then it's done in your computer settings. I did and for me the amounts there are with .. People with , as their preferred separator will have ,.

If you mean in the utils.displayAmount, yes we need to use the user's separator there, but as far as I know there is no simple way to do read this setting from the webpage. Maybe we can use JavaScript's toString inside the function somehow. Anyways it requires some time and fiddling with, I suggest we do it in a separate PR. #76

Yep, I meant in the displayAmount. The simplest way to get the values is to create a locale specific string of a number and that has a decimal and thousands separator and then just read the values from the string. There are several examples on stack-overflow. I will do a PR for this specifically quickly this morning.

fliebenberg · 2023-10-16T09:36:23Z

Created a separate issue #159 and PR for reading the separators and saving them in the global store. Somehow, the separators shown in the numbers is still not the same as the separator I need to put in for inputs. E.g. in South Africa, it shows numbers with a . as decimal separator, but yet in the input box it still only accepts a ,. I will need to investigate why this might be a bit more. Would be intersting if others can test it for their locales as well.

nguvictor

When trying to type 1.0 into the price of a pair, it seems to not allow it.
For example: I was trying to type out 1.01 as my limit price, but it always went back to 1. There are not issue with other numbers.

abdulhaseeb13mar added 5 commits September 14, 2023 01:02

feat: reusable input component

85840eb

feat: reusable token avatar component

56578aa

style: setting base style for html elements

4021278

package: install @tailwindcss/container-queries

dc77e9a

order input component polished and code revamp

eec05ae

abdulhaseeb13mar requested review from EvgeniiaVak, EngineerCharlie and maelsar September 14, 2023 17:03

EvgeniiaVak requested changes Sep 15, 2023

View reviewed changes

EvgeniiaVak previously requested changes Sep 18, 2023

View reviewed changes

EngineerCharlie and others added 3 commits September 20, 2023 11:04

Corrected setPercentage behaviour

2a9e02c

feat: merket order input functionality

6a206bf

revamp: limit order initial functionality

bbdaed9

update: hide balance,prevent default in limit only

9b4a4ed

Merge branch 'main' into order-input

7c39a38

abdulhaseeb13mar added 5 commits September 26, 2023 17:56

feat: ability to switch token in order input

7d0aefd

Merge branch 'order-input' of https://github.com/abdulhaseeb13mar/dex…

efbe191

…ter-frontend into order-input

update: replace heroIcons with react-icons

3ec3490

fix: prettier formatting in global.css

2c6fb6e

css: border-0 for input component

86a61aa

abdulhaseeb13mar commented Oct 13, 2023

View reviewed changes

EvgeniiaVak added 2 commits October 14, 2023 19:43

code cosmetic improvements

8b021e7

removes order type top margin

d1bdccb

fixes zero inputs

f855114

EvgeniiaVak requested review from nguvictor and fliebenberg and removed request for EvgeniiaVak, EngineerCharlie and maelsar October 15, 2023 07:35

aligns validation error text to the right

cfe3607

fixes cost calculation

151f696

nguvictor requested changes Oct 16, 2023

View reviewed changes

EvgeniiaVak added 3 commits October 18, 2023 13:27

fixes not being able to type in 0

5cdfea8

adds placeholder to amount input

4001b0c

fixes slippage state

1ea517b

nguvictor approved these changes Oct 18, 2023

View reviewed changes

EvgeniiaVak merged commit a5e287b into DeXter-on-Radix:main Oct 19, 2023

EvgeniiaVak mentioned this pull request Oct 19, 2023

Order button updates #72

Closed

10 tasks

abdulhaseeb13mar deleted the order-input branch October 21, 2023 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order input component style and code polishing #117

Order input component style and code polishing #117

abdulhaseeb13mar commented Sep 14, 2023 •

edited by EvgeniiaVak

Loading

netlify bot commented Sep 14, 2023 •

edited

Loading

EvgeniiaVak left a comment

EvgeniiaVak Sep 15, 2023

abdulhaseeb13mar Sep 15, 2023 •

edited

Loading

EvgeniiaVak Sep 15, 2023

abdulhaseeb13mar Sep 15, 2023

EvgeniiaVak Sep 15, 2023

EvgeniiaVak Sep 15, 2023

abdulhaseeb13mar Sep 15, 2023

EvgeniiaVak Sep 18, 2023

abdulhaseeb13mar commented Sep 15, 2023

EvgeniiaVak Sep 18, 2023

abdulhaseeb13mar commented Sep 23, 2023 •

edited

Loading

EvgeniiaVak commented Sep 24, 2023

abdulhaseeb13mar commented Sep 24, 2023 •

edited

Loading

abdulhaseeb13mar commented Sep 24, 2023

EvgeniiaVak commented Sep 25, 2023

EvgeniiaVak commented Sep 25, 2023

abdulhaseeb13mar commented Sep 25, 2023 •

edited

Loading

fliebenberg commented Sep 25, 2023

abdulhaseeb13mar Oct 13, 2023

fliebenberg commented Oct 14, 2023

fliebenberg commented Oct 14, 2023

EvgeniiaVak commented Oct 14, 2023

EvgeniiaVak commented Oct 14, 2023

nguvictor commented Oct 14, 2023

nguvictor commented Oct 14, 2023

fliebenberg commented Oct 15, 2023

EvgeniiaVak commented Oct 16, 2023

fliebenberg commented Oct 16, 2023

fliebenberg commented Oct 16, 2023

nguvictor left a comment

		const [customPercentage, setCustomPercentage] = useState(0);
		const [isFirstInteraction, setIsFirstInteraction] = useState(true);

Order input component style and code polishing #117

Order input component style and code polishing #117

Conversation

abdulhaseeb13mar commented Sep 14, 2023 • edited by EvgeniiaVak Loading

netlify bot commented Sep 14, 2023 • edited Loading

✅ Deploy Preview for chipper-sunburst-578cfe ready!

EvgeniiaVak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhaseeb13mar Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhaseeb13mar commented Sep 15, 2023

Choose a reason for hiding this comment

abdulhaseeb13mar commented Sep 23, 2023 • edited Loading

EvgeniiaVak commented Sep 24, 2023

abdulhaseeb13mar commented Sep 24, 2023 • edited Loading

abdulhaseeb13mar commented Sep 24, 2023

EvgeniiaVak commented Sep 25, 2023

EvgeniiaVak commented Sep 25, 2023

abdulhaseeb13mar commented Sep 25, 2023 • edited Loading

fliebenberg commented Sep 25, 2023

Choose a reason for hiding this comment

fliebenberg commented Oct 14, 2023

fliebenberg commented Oct 14, 2023

EvgeniiaVak commented Oct 14, 2023

EvgeniiaVak commented Oct 14, 2023

nguvictor commented Oct 14, 2023

nguvictor commented Oct 14, 2023

fliebenberg commented Oct 15, 2023

EvgeniiaVak commented Oct 16, 2023

fliebenberg commented Oct 16, 2023

fliebenberg commented Oct 16, 2023

nguvictor left a comment

Choose a reason for hiding this comment

abdulhaseeb13mar commented Sep 14, 2023 •

edited by EvgeniiaVak

Loading

netlify bot commented Sep 14, 2023 •

edited

Loading

abdulhaseeb13mar Sep 15, 2023 •

edited

Loading

abdulhaseeb13mar commented Sep 23, 2023 •

edited

Loading

abdulhaseeb13mar commented Sep 24, 2023 •

edited

Loading

abdulhaseeb13mar commented Sep 25, 2023 •

edited

Loading