style: implement new design for Place Order part in Trade path #34
Conversation
scorebreaker
force-pushed
the
design-trade
branch
2 times, most recently
from
ad7de03 to
09b8a1a
Compare
scorebreaker
force-pushed
the
design-trade
branch
from
09b8a1a to
5195dc6
Compare
There was a problem hiding this comment.
Good start, thanks! A few general comments:
- when narrowing the window width to < 1000px, a horizontal scrollbar appears;
- it seems that the font is not the same as in the designs;
- the price warning dialog does not attach to the place order box anymore. It should only cover the place order part instead of whole page;
- the place warning dialog has buttons with rounded corners (the sharp corners are applied only to
ButtonWithLoading?); - buttons (with loading?) have become way too large (see for instance a button in open orders table when an order is created);
- would really appreciate if color codes and other general styles would be inside the common components unless they are extremely specific. I guess we could use more generalization here and separate the styles that are used throughout the application in order to avoid duplicate code and make the code easier to maintain.
| backgroundColor: | ||
| props.color === "primary" | ||
| ? theme.palette.primary.main | ||
| : theme.palette.secondary.main, |
There was a problem hiding this comment.
Two concerns here:
- the value of color is not either primary or secondary, it can also be another valid option of Material Ui's button color. So let's keep it same as
props.color(withprimarydefault). Also, thecolorvalue should match with other possible button colors; - do we want this style only for
ButtonWithLoadingor also for other buttons?
| resetFormData={resetFormData} | ||
| setOrderError={setOrderError} | ||
| orderError={orderError} | ||
| tradeStore={tradeStore} |
There was a problem hiding this comment.
Let's inject the stores in the component instead of passing them as properties.
| @@ -19,25 +13,20 @@ const useStyles = makeStyles(() => { | |||
| display: "flex", | |||
| alignItems: "center", | |||
| }, | |||
| sectionTitle: { | |||
| color: "#f2f2f2", | |||
There was a problem hiding this comment.
Maybe would make sense to define the text colors in theme? Would be great not to define it in every component that uses this text color, and there seem to be many.
| textHighlighted: { | ||
| color: "#f2f2f2", | ||
| }, | ||
| toolTip: { |
There was a problem hiding this comment.
I suppose all the tooltips should look alike, so why not create a component that has those styles and can be used anywhere?
| color: "#979797", | ||
| fontSize: "16px", |
There was a problem hiding this comment.
It seems we could make use of text components that have sizes, colors, etc. Would eliminate duplicate code and make possible restyling and maintaining lot easier.
| <ThemeProvider theme={tradeTheme}> | ||
| <div className={classes.wrapper} id={id}> | ||
| <div className={classes.wrapperWithoutPlaceOrderButton}> | ||
| {!initialLoadCompleted || loadingErrors.size ? ( |
There was a problem hiding this comment.
Currently, the place order button is visible when those conditions are not met. It shouldn't.
| const useStyles = makeStyles((theme: Theme) => { | ||
| return createStyles({ | ||
| wrapper: { | ||
| maxWidth: "550px", |
There was a problem hiding this comment.
Why is there max width? Imo it does not look good like this. The open orders section does certainly not fit into 550px and would not make sense to keep those sections with different widths. So let's keep them dynamic.
Also, the place order box "jumps" (changes its height) when i switch between buy and sell. The reason is that I have different max limits and with 7 decimal numbers, the Max: 0.0292936 ETH does not fit into one row, causing the box to increase in height. At least 8 decimals have to fit into one row.
| root: { | ||
| padding: "8px 0", | ||
| }, | ||
| thumb: { |
There was a problem hiding this comment.
The thumb looks a bit off (it's downwards from the line + the white border is not continuous).
| import { TradeStore, TRADE_STORE } from "../../../stores/tradeStore"; | ||
| import Button from "../../../common/components/input/buttons/Button"; | ||
|
|
||
| const useStyles = makeStyles(() => ({ |
There was a problem hiding this comment.
The color of those buttons should only depend on whether buy or sell is selected. If buy, both are green; if sell, both are red.
Implement new design for Place Order part in Trade path.