WIP: Feature/styled components #24
Conversation
krrprr
left a comment
krrprr
left a comment
There was a problem hiding this comment.
Great job @dejankocev !
Found only one broken style - see in the comments.
Otherwise just minor nits:
Would be nice to get rid of the console warning that appears in two places.
Also added a few suggestions for adding more precise types.
The linter complains about single quote use in the imports. This should also be fixed.
|
|
||
| //types | ||
| type SortIconContainerType = { | ||
| item: any; |
There was a problem hiding this comment.
Could the item field be boolean like the container is or were there any issues with that?
| fullWidth: boolean; | ||
| disableRipple: boolean; | ||
| size: string; | ||
| onClick: any; |
There was a problem hiding this comment.
If possible, let's add a more specific () => void instead of any.
|
|
||
| //type | ||
| type StyledGridType = { | ||
| container: any; |
| type SnackbarType = { | ||
| open: boolean; | ||
| autoHideDuration: number; | ||
| onClose: any; |
There was a problem hiding this comment.
Let's add the () => void type if possible.
| } | ||
|
|
||
| export const ListItem = styled($ListItem)<ListItemType>` | ||
| ${p => p.isDisabled && ` |
There was a problem hiding this comment.
Currently there's a console error in dev mode.
Warning: React does not recognize the
isDisabledprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercaseisdisabledinstead. If you accidentally passed it from a parent component, remove it from the DOM element.
Not sure if it's necessary to add it to the DOM but if it's ok and there's no other good workaround, I suggest changing it to disabled instead.
| `; | ||
|
|
||
| export const SortOptionActiveGrid = styled(Grid)<SortOptionActiveGridType>` | ||
| ${p => p.sortOptionActive && ` |
|
|
||
| const getArrowIconStyles = (p: any) => { | ||
| return ` | ||
| font-size: ${p.theme.typography.body2.fontSize}px; |
There was a problem hiding this comment.
Seems that the body2.fontSize value already includes a unit, so that currently it appears as font-size: 0.875rempx; and breaks the style.
krrprr
left a comment
krrprr
left a comment
There was a problem hiding this comment.
Code-wise looks good.
Remaining issues:
- I would suggest adding types where possible.
- The console warnings for isDisabled and sortOptionActive still persist.
- The linter error needs fixing.
Also, a new thing is that the dark theme and drawer menu width seem to not get fully applied, and the disconnect button is misplaced:
| } | ||
|
|
||
| type AddressFieldType = { | ||
| fullWidth: any; |
There was a problem hiding this comment.
Would prefer to give as many exact types as possible. I guess here at least fullWidth: boolean and value: string would make sense.
Introducing styled-components and updating project structure and components according to it