-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Address book Page #38
Conversation
bogos
commented
Aug 31, 2023
•
edited
Loading
edited
- Added AddressBook and view route
- Added AddressBook tables component
- Added a modal to add new AddressBook records
- Hooks were created to handle the address book CRUD logic,
- Updated the AddressBookWidget to read the data with the new read hook
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…d delete address books
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is ready to merge, however I would improve the mentioned aspects.
src/pages/app/address-book.tsx
Outdated
@@ -0,0 +1,5 @@ | |||
import { AddressBookPage } from "@/components/AddressBook"; | |||
|
|||
export default function AddressBook() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component would be the AddressBookPage, while the nested one would be the AddressBookContainer
/ AddressBookView
.
As the AddressBookPage
is closer to the response it is preferable that the page have the title and be responsible of handling the critical hooks like network
and pass it to the presentation layer the AddressBookView
.
const mockURL = "https://polkadot.subscan.io/"; | ||
|
||
const AddressBookTable = () => { | ||
const { network } = usePolkadotContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you handling hooks for data I/O here if you have a parent Component where you use the same hooks usePolkadotContext
and useFetchAddressBook
?
import SvgIconButton from "../common/SvgIconButton"; | ||
import NetworkBadge from "../NetworkBadge"; | ||
|
||
// TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please 🙏
Add a new task to work in this later.
} | ||
|
||
export const AddressBookItem = ({ addressBook, network, key }: Props) => { | ||
export const AddressBookItem = ({ addressBook, network }: Props) => { | ||
// TODO: | ||
// Remove this mock variable, replace with true value | ||
const mockURL = "https://polkadot.subscan.io/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a repeat.
@@ -5,7 +5,7 @@ import * as React from "react"; | |||
import { ChainExtended, getChain } from "@/config/chain"; | |||
import { ROUTES } from "@/config/routes"; | |||
import { usePolkadotContext } from "@/context/usePolkadotContext"; | |||
import { useListAddressBook } from "@/hooks/addressBook/useListAddressBook"; | |||
import { useFetchAddressBook } from "@/hooks/addressBook/useFetchAddressBook"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By curiosity, I liked List
better, why did you change it to fetch?
I associate fetch more with http external request.
src/domain/AddressBooks.ts
Outdated
@@ -4,4 +4,12 @@ export interface AddressBook { | |||
address: string; | |||
name: string; | |||
networkId: Chain["id"]; | |||
isEditable: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain this attribute.
🤔 Why wouldn't it be an address editable?
tempData[index] = element; | ||
addressBookRepository.saveAddress(tempData); | ||
document.dispatchEvent( | ||
new CustomEvent(AddressBookEvents.onFetchAddressBook) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the event fired after saving the new address be onAddressUpdated
?
saveAddress(account: AddressBook): void { | ||
setLocalStorageState(this.storageKey, account); | ||
deleteAddress(accountAddress: string): void { | ||
const data = getLocalStorageState<AddressBook[] | null>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no use getData
here?
const filter = Object.values(data).filter( | ||
(element) => element.address !== accountAddress | ||
); | ||
console.log("data", filter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose to use this console here?