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

Simple readonly top 100 addresses prototype address book and new way to move data between backround page and popups #88

Merged
merged 7 commits into from
Jan 5, 2023

Conversation

KillariDev
Copy link
Contributor

  • Adds address book that opens into a new tab
  • The address book is view only at the moment, and only shows top 100 addresses for each category
  • Address book communicates with backgroundpage by sending messages, instead of fetching window.interceptor, the plan is to change all popups to communicate like this, so we can get rid of the global window.interceptor eventually

image

@KillariDev KillariDev added the enhancement New feature or request label Jan 4, 2023
"funtypes": "../vendor/funtypes/index.mjs",
"node-fetch": "../vendor/node-fetch/index.mjs",
"@zoltu/ethereum-abi-encoder": "../vendor/@zoltu/ethereum-abi-encoder/index.js",
"@zoltu/ethereum-crypto": "../vendor/@zoltu/ethereum-crypto/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider moving away from @zoltu/ethereum-crypto. There may also be better alternatives to rlp-encoder and abi-encoder as well that could be worth investigating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created an issue to look into that

<main>Loading...</main>

<!-- a workaround for bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1803984 -->
<script type = 'module'>import '../js/addressBookRender.js'</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script type = 'module'>import '../js/addressBookRender.js'</script>
<script async type = 'module'>import '../js/addressBookRender.js'</script>

</a>
}

export function AddressList(param: { addressBookEntries: AddressBookEntries | undefined }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider destructuring here so you don't have to do param. everywhere.

Suggested change
export function AddressList(param: { addressBookEntries: AddressBookEntries | undefined }) {
export function AddressList({addressBookEntries}: { addressBookEntries: AddressBookEntries | undefined }) {

const [addressBookEntries, setAddressBookEntries] = useState<AddressBookEntries | undefined>(undefined)
const activeFilterRef = useRef<ActiveFilter>(activeFilter)

useEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style consistency I think? You have been doing this a lot lately, maybe it is intentional and the goal is to change the style to include a space here? I'm not a big fan because it goes against the general style of "no space after parenthesis", but I'm open to having my mind changed.

Suggested change
useEffect( () => {
useEffect(() => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is not intentiol :D

@KillariDev KillariDev merged commit 26a0965 into main Jan 5, 2023
@KillariDev KillariDev deleted the addressbook branch January 5, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants