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

Initial Proof of Concept UI #1

Merged
merged 53 commits into from
Mar 12, 2021
Merged

Conversation

matt-mertens
Copy link
Contributor

General layout and application shell for the UI created as well as provide an interface for many of the debug / files API endpoints

Copy link
Contributor

@vojtechsimetka vojtechsimetka left a comment

Choose a reason for hiding this comment

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

Great work so far! I really like it as a general skeleton. If I can ask you, please focus mostly on the Status tab for now. We really want it to be something that guides new users when setting up bee and can alert them on problem. I don't want to impose my ideas, but I'd like to share my very rudimental idea (feel free to do it as you want!). I imagined it as the Vertical Stepper component which would detect which steps are already done and which is the one user still needs to do as per the original assignment.

I know this is WIP, but I'll share few things that I noticed:

  • The SideBardoes not show active element
  • Status page is mapped to both / and /status
  • You often have fetch function and state variable where you store the result. This can be extracted into custom hook which would be more readable and easier to reuse https://reactjs.org/docs/hooks-custom.html
  • Try and use the corresponding bee-js function calls rather than custom Axios. If you are missing any functionality let us know and we build it :) .

Also a comment for myself - add linter and linting rules. We usually use rules like interfaces should not be prepended with I, no semicolon at the end of command, no functions starting with upper case or some of the spacing. Don't worry about this though - we can solve linter later.

package.json Outdated Show resolved Hide resolved
public/index.html Outdated Show resolved Hide resolved
src/components/NavBar.tsx Outdated Show resolved Hide resolved
src/pages/accounting/AccountCard.tsx Outdated Show resolved Hide resolved
Comment on lines 33 to 49
const [chequebookAddress, setChequebookAddress] = useState({ chequebookaddress: '' });
const [loadingChequebookAddress, setLoadingChequebookAddress] = useState(false);

const [chequebookBalance, setChequebookBalance] = useState({ totalBalance: 0, availableBalance: 0});
const [loadingChequebookBalance, setLoadingChequebookBalance] = useState(false);

const [peerBalances, setPeerBalances] = useState({ balances: [{peer: '-', balance: 0 }] });
const [loadingPeerBalances, setLoadingPeerBalances] = useState(false);

const [peerCheques, setPeerCheques] = useState({ lastcheques: [{peer: '-', lastsent: {beneficiary: '', chequebook: '', payout: 0}, lastreceived: {beneficiary: '', chequebook: '', payout: 0} }] });
const [loadingPeerCheques, setLoadingPeerCheques] = useState(false);

const [nodeAddresses, setNodeAddresses] = useState({ overlay: '', underlay: [""], ethereum: '', public_key: '', pss_public_key: ''});
const [loadingNodeAddresses, setLoadingNodeAddresses] = useState(false);

const [nodeSettlements, setNodeSettlements] = useState({ totalreceived: 0, totalsent: 0, settlements: [{peer: '-', received: 0, sent: 0}] });
const [loadingNodeSettlements, setLoadingNodeSettlements] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be typed just to avoid type mistakes. E.g. one of these

import type { PeerBalance, BalanceResponse } from '@ethersphere/bee-js'

const [peerBalances, setPeerBalances] = useState<BalanceResponse>({ balances: [{peer: '-', balance: 0 }] });

// OR

const [peerBalances, setPeerBalances] = useState<PeerBalance[]>([{peer: '-', balance: 0 }]);

Btw it also likely can be just an empty array :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah now that I know that beeJS has all the types included I agree. I dont have a ton of experience with Typescript so I wasnt really sure how to handle this situation. Theres defintely a few places where I used types of "any" that'll need to be fixed.

Comment on lines 6 to 16
const beeApiClient = (): AxiosInstance => {
return axios.create({
baseURL: process.env.REACT_APP_BEE_HOST
})
}

const beeDebugApiClient = (): AxiosInstance => {
return axios.create({
baseURL: process.env.REACT_APP_BEE_DEBUG_HOST
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be replaced by bee-js calls on top of Bee resp BeeDebug class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah Im just getting familar with beeJS so I wasn't too sure of all the functionality. I just realized all the types for bee were already in the library! Should have read through the documentation more haha.

I removed the beeJS api client but kept the debug one though since I dont see all the APIs for the debug on beeJS.

Im leaving it wrapped in the "service" for now because I wasnt sure how stable the beeJS library API was and think its easier to update API changes from the single place. I think once the beeJS library is stable and out of beta this can definitely be removed.

@vojtechsimetka
Copy link
Contributor

Few more UX suggestions:

Screenshot 2021-03-07 at 16 43 57

  • make the steps clickable so that users can skip back and forth more easily
  • I would consider overwriting the numbers with status icon or just keep the numbers and put the status icon in front of the step name
  • the troubleshoot text is only for linux. That is fine for now, but please think about how this could be separated into different supported OSes. Also having OS detection would be nice

Screenshot 2021-03-07 at 16 51 52
-Consider making the Ethereum Swarm footer non-fixed or putting it to the left column

  • In general if we can shave off some margins would be nice so that it is less likely to overflow

  • Not sure if already implemented but several things for me don't work - dark mode, wallet connection, the copy on ETH address etc

@matt-mertens
Copy link
Contributor Author

Thanks for the feedback.

  1. make the steps clickable so that users can skip back and forth more easily
  2. I would consider overwriting the numbers with status icon or just keep the numbers and put the status icon in front of the step name

100% agree with both of the above suggestions, will plan on adding

  1. the troubleshoot text is only for linux. That is fine for now, but please think about how this could be separated into different supported OSes. Also having OS detection would be nice

Agree, for now I plan on just getting it up for linux but would appreciate input from others to make it more helpful for all OS's

@matt-mertens
Copy link
Contributor Author

-Consider making the Ethereum Swarm footer non-fixed or putting it to the left column

^^^ Ive been playing around with this an cannot get it to function properly

Not sure if already implemented but several things for me don't work - dark mode, wallet connection, the copy on ETH address etc

Dark mode - currently it works off device preference detection, just need to add the last bit of enabling control with the navbar button

Wallet Connection - placeholder for now but was intending to make it so that you can connect a wallet to directly fund the node from the UI to help with a more seemless setup process

Copy on ETH address - this is currently working for me, perhaps open an issue and note any errors your seeing

src/App.tsx Outdated Show resolved Hide resolved
src/pages/status/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@vojtechsimetka vojtechsimetka left a comment

Choose a reason for hiding this comment

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

Hey, great work! Let's aim to merge this, but before we do so:

  • There is a lot of warnings in the compilation of unused variables etc. Can we clean those up?
  • I am also confused by the amount of logos, can this also be cleaned up?
  • Please remove all the functionality that does not exist - e.g. file uploading. We can do this in some other PR :)

We did a thorough review today with the team and identified number of improvements we'd like to see. I'm listing here those that I would like to see implemented in this PR:

src/App.tsx Outdated Show resolved Hide resolved
src/App.tsx Outdated Show resolved Hide resolved
src/components/CashoutModal.tsx Outdated Show resolved Hide resolved
Comment on lines +65 to +99
const AntTab = withStyles((theme: Theme) =>
createStyles({
root: {
textTransform: 'none',
minWidth: 72,
backgroundColor: 'transparent',
fontWeight: theme.typography.fontWeightRegular,
marginRight: theme.spacing(4),
fontFamily: [
'-apple-system',
'BlinkMacSystemFont',
'"Segoe UI"',
'Roboto',
'"Helvetica Neue"',
'Arial',
'sans-serif',
'"Apple Color Emoji"',
'"Segoe UI Emoji"',
'"Segoe UI Symbol"',
].join(','),
'&:hover': {
color: '#3f51b5',
opacity: 1,
},
'&$selected': {
color: '#3f51b5',
fontWeight: theme.typography.fontWeightMedium,
},
'&:focus': {
color: '#3f51b5',
},
},
selected: {},
}),
)((props: StyledTabProps) => <Tab disableRipple {...props} />);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is extremely inefficient, can you externalize it into a separate component?

src/components/WithdrawlModal.tsx Outdated Show resolved Hide resolved
src/hooks/apiHooks.tsx Outdated Show resolved Hide resolved
Comment on lines +76 to +110
const AntTab = withStyles((theme: Theme) =>
createStyles({
root: {
textTransform: 'none',
minWidth: 72,
backgroundColor: 'transparent',
fontWeight: theme.typography.fontWeightRegular,
marginRight: theme.spacing(4),
fontFamily: [
'-apple-system',
'BlinkMacSystemFont',
'"Segoe UI"',
'Roboto',
'"Helvetica Neue"',
'Arial',
'sans-serif',
'"Apple Color Emoji"',
'"Segoe UI Emoji"',
'"Segoe UI Symbol"',
].join(','),
'&:hover': {
color: '#3f51b5',
opacity: 1,
},
'&$selected': {
color: '#3f51b5',
fontWeight: theme.typography.fontWeightMedium,
},
'&:focus': {
color: '#3f51b5',
},
},
selected: {},
}),
)((props: StyledTabProps) => <Tab disableRipple {...props} />);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please externalize this into separate component. Also I believe something like that already exists, right?

src/pages/files/index.tsx Outdated Show resolved Hide resolved
src/components/EthereumAddressCard.tsx Outdated Show resolved Hide resolved
src/pages/accounting/AccountCard.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@vojtechsimetka vojtechsimetka left a comment

Choose a reason for hiding this comment

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

Went over it again, and it is quite good. So I will merge and let's tackle the improvements as individual PRs.

Comment on lines +74 to +80
window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', e => {
toggleThemeMode(e.matches ? "dark" : "light")
});

return () => window.matchMedia('(prefers-color-scheme: dark)').removeEventListener('change', e => {
toggleThemeMode(e.matches ? "dark" : "light")
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing event listener needs the original function to be passed. Basically you are just passing in a different object to be removed.

Suggested change
window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', e => {
toggleThemeMode(e.matches ? "dark" : "light")
});
return () => window.matchMedia('(prefers-color-scheme: dark)').removeEventListener('change', e => {
toggleThemeMode(e.matches ? "dark" : "light")
})
const changeHandle = e => toggleThemeMode(e.matches ? "dark" : "light")
window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', changeHandle);
return () => window.matchMedia('(prefers-color-scheme: dark)').removeEventListener('change', changeHandle)

Easy way to demonstrate is:

// This removes the listener
const t = () => console.log('resize removed')

window.addEventListener('resize', t)
window.removeEventListener('resize', t)

// This removes the listener
window.addEventListener('resize', () => console.log('resize not removed'))
window.removeEventListener('resize', () => console.log('resize not removed'))

Comment on lines +26 to +45
var userAgent = window.navigator.userAgent,
platform = window.navigator.platform,
macosPlatforms = ['Macintosh', 'MacIntel', 'MacPPC', 'Mac68K'],
windowsPlatforms = ['Win32', 'Win64', 'Windows', 'WinCE'],
iosPlatforms = ['iPhone', 'iPad', 'iPod'],
os = null;

if (macosPlatforms.indexOf(platform) !== -1) {
os = 'macOS';
} else if (iosPlatforms.indexOf(platform) !== -1) {
os = 'iOS';
} else if (windowsPlatforms.indexOf(platform) !== -1) {
os = 'windows';
} else if (/Android/.test(userAgent)) {
os = 'android';
} else if (!os && /Linux/.test(platform)) {
os = 'linux';
}

return os;
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit more readable :)

Suggested change
var userAgent = window.navigator.userAgent,
platform = window.navigator.platform,
macosPlatforms = ['Macintosh', 'MacIntel', 'MacPPC', 'Mac68K'],
windowsPlatforms = ['Win32', 'Win64', 'Windows', 'WinCE'],
iosPlatforms = ['iPhone', 'iPad', 'iPod'],
os = null;
if (macosPlatforms.indexOf(platform) !== -1) {
os = 'macOS';
} else if (iosPlatforms.indexOf(platform) !== -1) {
os = 'iOS';
} else if (windowsPlatforms.indexOf(platform) !== -1) {
os = 'windows';
} else if (/Android/.test(userAgent)) {
os = 'android';
} else if (!os && /Linux/.test(platform)) {
os = 'linux';
}
return os;
const userAgent = window.navigator.userAgent
const platform = window.navigator.platform
const macosPlatforms = ['Macintosh', 'MacIntel', 'MacPPC', 'Mac68K']
const windowsPlatforms = ['Win32', 'Win64', 'Windows', 'WinCE']
const iosPlatforms = ['iPhone', 'iPad', 'iPod']
if (macosPlatforms.includes(platform)) return 'macOS'
if (iosPlatforms.includes(platform)) return 'iOS'
if (windowsPlatforms.includes(platform)) return 'windows'
if (/Android/.test(userAgent)) return 'android'
if (/Linux/.test(platform)) return 'linux'
return null;

Comment on lines +100 to +105
<div className={classes.details}>
<Skeleton width={180} height={110} animation="wave" style={{ marginLeft: '12px', marginRight: '12px'}} />
<Skeleton width={180} height={110} animation="wave" style={{ marginLeft: '12px', marginRight: '12px'}} />
<Skeleton width={180} height={110} animation="wave" style={{ marginLeft: '12px', marginRight: '12px'}} />
<Skeleton width={180} height={110} animation="wave" />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

The style should be extracted into classname with lastchild selector.

function truncStringPortion(str: string, firstCharCount=10, endCharCount=10) {
var convertedStr="";
convertedStr+=str.substring(0, firstCharCount);
convertedStr += ".".repeat(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha this is a bit excessive :). Also it should probably be a template string rather than operation on string

Comment on lines +90 to +96
useEffect(() => {
evaluateNodeStatus()
}, [])

useEffect(() => {
evaluateNodeStatus()
}, [props])
Copy link
Contributor

Choose a reason for hiding this comment

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

there should probably be some isLoading state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants