Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Design for Table component on Farms page #482

Merged
merged 76 commits into from
Mar 3, 2021

Conversation

plind-dm
Copy link
Contributor

@plind-dm plind-dm commented Feb 18, 2021

Fix #360
Fix #511

@plind-dm plind-dm marked this pull request as ready for review February 24, 2021 21:19
@@ -17,13 +17,36 @@ interface FarmCardActionsProps {
addLiquidityUrl?: string
}

const StyledHeading = styled(Heading)`
margin-right: 48px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a component for this, you can use the prop mr on Heading

end: earningsBusd,
duration: 1,
separator: ',',
// eslint-disable-next-line no-nested-ternary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this please

export const Staked = styled.div`
font-size: 12px;
color: ${(props) => props.theme.colors.textSubtle};
`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file hasn't been prettified

info: string
}

const StyledLinkExternal = styled(LinkExternal)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't want to display the svg, use the Link component , with the external props

const TranslateString = useI18n()

return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary fragment

@@ -0,0 +1,94 @@
import React, { useRef } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this component is here and not in the uikit ?


import { ActionContainer, ActionTitles, Title, Subtle, ActionContent, Earned, Staked } from './styles'

const HarvestButton = styled(Button)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to create a new component for this.
Most of the uikit component use the space API from styled-system
which means you can use <Button ml="4px">....</Button> instead of this

@@ -15,8 +15,13 @@ const ApyButton: React.FC<ApyButtonProps> = ({ lpLabel, cakePrice, apy, addLiqui
<ApyCalculatorModal lpLabel={lpLabel} cakePrice={cakePrice} apy={apy} addLiquidityUrl={addLiquidityUrl} />,
)

const handleClickButton = (event): void => {
event.stopPropagation()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Comment on lines 30 to 36
button {
width: 32px;
height: 32px;

svg {
width: 20px;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not use the <IconButton /> component?


const StyledInput = styled(Input)`
border-radius: 16px;
transition: 0.3s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this transition for? Try and avoid setting transition for every property.

Comment on lines 9 to 12
${({ theme }) => theme.mediaQueries.sm} {
width: 100% !important;
padding: 0 1rem !important;
opacity: 1 !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please double check why !important is necessary. In principal if we need an !important on our own code then we did something wrong to begin with.


const ListItem = styled.li`
list-style: none;
padding: 6px 16px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Padding should be a multiple of 4px, please double check.

Comment on lines 120 to 121
const bsc = `https://bscscan.com/address/${farm.lpAddresses[process.env.REACT_APP_CHAIN_ID]}`
const info = `https://pancakeswap.info/pair/${farm.lpAddresses[process.env.REACT_APP_CHAIN_ID]}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put the chain id in a variable at the top of the file if we are using it more than once.

Comment on lines 70 to 72
if (account) {
if (isApproved) {
if (rawStakedBalance) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a cleaner way to represent this. Avoid using many nested if blocks. Maybe before this block you can do something like

if (!account) {
 return (
    <ActionContainer>
      <ActionTitles>
        <Subtle>{TranslateString(999, 'START FARMING')}</Subtle>
      </ActionTitles>
      <ActionContent>
        <UnlockButton fullWidth />
      </ActionContent>
    </ActionContainer>
    )
  }

}
`

const Tags: React.FunctionComponent<FarmWithStakedValue> = ({ tokenSymbol, dual }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React.FC

bottom: calc(100% + 16px);
transform: translate(34px, 0);
right: 0;
max-width: 246px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This number seems pretty arbitrary. It is okay for now but please try and void numbers like these.

@hachiojidev hachiojidev merged commit 589ca6c into develop Mar 3, 2021
@hachiojidev hachiojidev deleted the feature/farms-table-new branch March 3, 2021 05:26
taalswap pushed a commit to taalswap/taalswap-frontend that referenced this pull request Sep 3, 2021
* feat: create table component for farm page

* feat: add scrollbar option and complete header

* chore: Prettier

* wip

* feat: complete farm table

* Update Cell.tsx

fix: add defaultProps for Cell component

* feat: integrate API into table

* feat: table mobile

* style: lint

* style: lint

* build: update table component pack

* sort

* feat: call-to-action

* feat: black theme table

* feat: search & toggle

* feat: sticky table header and coin column

* feat: mobile search

* fix: uncomment

* wip

* conflicts resolved

* Wip

* build: Uikit package updated

* fix: Sort

* fix: Apr styling

* build: Uikit

* build: Uikit revert

* build: Yarn lock

* fix: Styling

* refactor: gitignore

* build: Revert npm packages

* style: Package.json

* style: Rm comment

* styling: Rem to px

* styling: Lowered to loweredcaseQuery

* styling: Tags

* fix: Rm unused styling

* feat: farm header

* feat: Mobile view

* feat: Select component

* Conflicts resolved

* feat: New table

* feat: Multiplier styling

* feat: Update select to use theme color

* style: Rm unused component

* feat: Localization

* style: Rm unused

* style: Rem to px

* style: Rm unused

* style: Rm unused

* feat: Select updated

* feat: Select update

* feat: Select arrow prerender

* feat: Smaller viewport row

* fix: Ios styling fixed

* build: Uikit update

* feat: Responsive styling

* feat: Select transition

* build: Uikit updated

* fix: Actions spacing on smaller viewport

* fix: Responsive styling

Co-authored-by: CBunnyDev <59729252+ranon127@users.noreply.github.com>
Co-authored-by: hachiojidev <hachiojidev@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QA Results: #482 (Tableview) Farms Table QA
4 participants