[T3111 ] Adding the navbar component. I seperated out the actual nav items and links to a seperate document for readability and because it will be used for the footer
Details
- Reviewers
bytesofman Mengerian - Group Reviewers
Restricted Project - Commits
- rABC02fd82f36b96: [e.cash] Add navbar component
run the app and see how it looks and performs. It is fully responsive as well so check that out
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
web/e.cash/components/navbar.js | ||
---|---|---|
27–30 ↗ | (On Diff #39556) | why is this block not included in the earlier useEffect for on page load? |
30 ↗ | (On Diff #39556) | Is this meant to occur on startup? Shouldn't it include an empty parameter? Has that syntax changed? e.g. the below useEffect runs when windowHeight changes, a useEffect intended to run on page load should include ,[]); |
50 ↗ | (On Diff #39556) | seems like this is the third "on load" useEffect -- everything intended to happen on page load should be in the same useEffect |
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://51.178.130.230:41115 for the next 60 minutes.
Personally I wouldn't put the menu items and links in yet. Right now the links don't work.
They can be added in as each page is added. It would be more robust to check each link as the associated page added in.
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://54.39.19.73:41067 for the next 60 minutes.
web/e.cash/components/navbar.js | ||
---|---|---|
8 ↗ | (On Diff #39635) | Since we are using this state field as a flexible string, and it need not necessarily be price, should be renamed priceLinkText, setPriceLinkText Otherwise it's not immediately clear why price is initialized as Buy XEC |
33 ↗ | (On Diff #39635) | getPrice is a synchronous function making an API call. If coingecko is down, the rest of the useEffect loop will not be reached until the request times out (not sure what this default is, but perhaps a few seconds). getPrice should be the last item called in this useEffect() |
web/e.cash/components/navbar.module.css | ||
1 ↗ | (On Diff #39635) | why .css and not a framework / library like styled-components? It's okay to go this way, but please include some justification / explanation. If the plan is to later implement some kind of library, we should do it now. |
web/e.cash/pages/index.js | ||
5 ↗ | (On Diff #39635) | why is this in-line css required? this should be handled by a styled component, class, or ID. |
Okay addressed some of those comments. Will patch others in a sec
web/e.cash/components/navbar.module.css | ||
---|---|---|
1 ↗ | (On Diff #39635) | Sure, main reasons I went for straight css
|
web/e.cash/pages/index.js | ||
5 ↗ | (On Diff #39635) | Well the style is there to add some scroll to the page so you can test the navbar background change...it will be temporary no matter what and I figured best place to put it is the html that will also be replaced with actual page content, as opposed to hunting down a class later |
create folder for componet/stylesheet, move getprice to last call, rename price variable
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://54.39.104.128:41677 for the next 60 minutes.
web/e.cash/components/navbar/index.js | ||
---|---|---|
45 ↗ | (On Diff #39642) | Would be useful to have some comments here explaining what this is doing |
171 ↗ | (On Diff #39642) | This site doesn't exist yet |
web/e.cash/pages/index.js | ||
5 ↗ | (On Diff #39642) | I can only see this text immediately after scrolling, in the top left, behind the nav bar. Doesn't seem like we will want anything to appear there? We don't want to use inline styles for anything. If it's temp and just for testing, you can write it in the test plan to add this type of code. In this case, better approach would be to not add the effect until it can be seen. |
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://51.178.130.230:41741 for the next 60 minutes.