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

refactor: update homepage to typescript and hooks #921

Merged
merged 26 commits into from
Feb 1, 2024

Conversation

ckniffen
Copy link
Collaborator

@ckniffen ckniffen commented Jan 20, 2024

High Level Overview of Change

Update homepage to typescript and hooks and break out aspects of the homepage's scrolling ledgers into smaller components. This also adds a new hook useTooltip that cleans up tooltip logic when many places are updating tooltips.

A small change to move the transaction iteration into its own component reduced render times of the homepage due by between 30ms to 60ms each re-render.

Type of Change

  • Refactor (non-breaking change that only restructures code)

TypeScript/Hooks Update

  • Updated files to React Hooks
  • Updated files to TypeScript

Future Work

Optimize the component into a hook that causes way fewer re-renders.

</b>
</div>
)}
<LedgerEntryTransactions transactions={ledger.transactions} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<LedgerEntryTransactions transactions={ledger.transactions} />
<LedgerEntryTransactions transactions={ledger.transactions || []} />

Copy link
Collaborator Author

@ckniffen ckniffen Jan 23, 2024

Choose a reason for hiding this comment

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

I chose not to always make it an array to differentiate between 0 transactions and it not loaded in.

@ckniffen ckniffen requested a review from mvadari January 24, 2024 18:39
Copy link
Collaborator

@khancode khancode left a comment

Choose a reason for hiding this comment

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

LGTM 👍


Object.keys(validators).forEach((pubkey) => {
if (validators[pubkey].unl) {
unl[pubkey] = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we set this to false instead of the unl value? It seems similar to undefined and I took a second to re-read to confirm that false just seems to be a sentinel value that's main trait is that it is NOT undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I honestly dont know this code was written this way before I just broke it out into its own component. I did rewrite it in the streams refactor though to this

const status = useMemo(() => {
    const missing = [...(unl || [])]
    validations.forEach((v) => {
      if (!validators?.[v.validation_public_key]) {
        return
      }

      const missingIndex = missing.findIndex(
        (assumedMissing) => assumedMissing === v.validation_public_key,
      )
      if (missingIndex !== -1) {
        missing.splice(missingIndex, 1)
      }
    })

    return {
      missing: missing.map((v) => validators?.[v]),
      trustedCount: (unl?.length || 0) - missing.length,
    }
  }, [unl, validations])

Base automatically changed from refactor/tooltip-tsx-hooks to staging January 29, 2024 23:43
# Conflicts:
#	src/containers/Ledgers/LedgerMetrics.jsx
#	src/containers/Ledgers/Ledgers.jsx
#	src/containers/Network/Hexagons.jsx
#	src/containers/PayStrings/PayStringHeader/index.tsx
}

export const LedgerMetrics = ({
data: suppliedData,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where suppliedData is defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

data is the props name but the linter prevents redefining a prop so i name it differently locally scoped to the function to make it shorter instead of having a longer name ever where i reference all the metrics

onPause: any
paused: boolean
}) => {
const data = { ...DEFAULTS, ...suppliedData }
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this cause a name collision with the input parameter data for LedgerMetrics? I'm a bit confused on the naming here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See previous comment

let content: any = null

let className = 'label'
if (data[key] === undefined && key !== 'nUnl') {
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 add a comment explaining why nUnl is special here? It's getting a lot of special treatment in this if/else - maybe there's a better way to separate out that logic? (ie. similar to how you deal with base_fee beforehand then delete it from the object?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has special rules i think because it has a link to another page and a tooltip. This code existed before this refactor.

>
<a
key={`link ${key}`}
href="https://xrpl.org/negative-unl.html"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this link be "localized" to point to the proper language if the explorer is in Japanese for example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't link anywhere else based on the language. I wouldn't know where to start since every site might have different rules like localstorage based or path based.

Copy link
Contributor

@JST5000 JST5000 left a comment

Choose a reason for hiding this comment

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

One open question, but otherwise looks good to me :)

@pdp2121 pdp2121 merged commit 77c8722 into staging Feb 1, 2024
4 checks passed
@pdp2121 pdp2121 deleted the refactor/homepage-tsx branch February 1, 2024 18:43
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.

None yet

5 participants