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

AK-14 - Added page titles #306

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

AK-14 - Added page titles #306

wants to merge 5 commits into from

Conversation

lempira
Copy link

@lempira lempira commented Oct 30, 2024

I looks through all of the URL routes and added a hook that creates the title based on the params of page. Here are the pages and examples of titles. I still have to write a test for the hook but wanted get feedback on approach and format before I write the test.

Pages:
"Index": "/",
Example: Lora Explore - mainnet

"Explore": "/:networkId",
Example: Lora Explore - mainnet

"Explore_Transaction_ById": "/:networkId/transaction/:transactionId",
Example: Lora - TxnId:CFNKY4JV5BMIRSP2SMZE634DYB22FALLURI72U6UMWZNKEBOYEMQ - mainnet

"Explore_Transaction_ById_Inner_ById": "/:networkId/transaction/:transactionId/inner//*",
Example: Lora - TxnId:Q36W3E5GBCPZYGEVFQGTQB4KX7QTFYV3H4UYNRYS4AFXEPILHQXQ, Inner:1 - mainnet

"Explore_Block_ByRound": "/:networkId/block/:round",
Example: Lora - Block:43904244 - mainnet

"Explore_Block_ByRound_Group_ById": "/:networkId/block/:round/group/:groupId",
Example: Lora - Block:43904244 - Group:iUE01XrzcJZ+ENIwPyjPmtaVUF5OKy8vYYOwGbdKliQ= - mainnet

"Explore_Account_ByAddress": "/:networkId/account/:address",
Example: Lora - Addr:W2IZ3EHDRW2IQNPC33CI2CXSLMFCFICVKQVWIYLJWXCTD765RW47ONNCEY - mainnet

"Explore_Asset_ById": "/:networkId/asset/:assetId",
Example: Lora - AssetId:1014 - localnet

"Explore_Application_ById": "/:networkId/application/:applicationId",
Example: Lora - AppId:1019 - localnet

"Explore_Tx": "/:networkId/tx/*",
Example: Lora - TxnId:CFNKY4JV5BMIRSP2SMZE634DYB22FALLURI72U6UMWZNKEBOYEMQ - mainnet

"Explore_Txn": "/:networkId/txn/*",
Example: Lora - TxnId:CFNKY4JV5BMIRSP2SMZE634DYB22FALLURI72U6UMWZNKEBOYEMQ - mainnet

"AppLab": "/app-lab",
Example: Lora App Lab

"AppLab_Create": "/app-lab/create",
Example: Lora Create App Interface

"Settings": "/settings",
Example: Lora Settings

"Fund": "/fund",
Example: Lora Fund

"FundAuthCallback": "/fund/auth-callback",
Example: NA

"TransactionWizard": "/transaction-wizard",
Example: Lora Transaction Wizard

"TxWizard": "/tx-wizard",
Example: Lora Transaction Wizard

"TxnWizard": "/txn-wizard"
Example: Lora Transaction Wizard

Copy link
Contributor

@neilcampbell neilcampbell left a comment

Choose a reason for hiding this comment

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

The approach looks good to me 👍

document.title = title
}

export const useTitle = (customString?: string) => {
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
export const useTitle = (customString?: string) => {
export const useTitle = (pagePrefix?: string) => {

My thought process here is that anything after the Lora - is the page specific portion of the title, so it's a page prefix, even though it's not the title prefix.

const urlParams = useParams()
useEffect(() => {
const currentTitle = document.title
let pageTitle = `Lora`
Copy link
Contributor

@neilcampbell neilcampbell Oct 30, 2024

Choose a reason for hiding this comment

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

I think it would be good for all pages to start with Lora - ${pageTitle}
So:
Lora - Transaction Wizard
Lora - TxnId:CFNKY4JV5BMIRSP2SMZE634DYB22FALLURI72U6UMWZNKEBOYEMQ - mainnet

Probably the easiest way to do this is accumulate in an array, then .join(' - ') to build the page title. Something like below:

  const params = new Array<string>()

  if (urlParams.transactionId) {
    params.push(`TxnId:${urlParams.transactionId}`)
  }
  // ...

  const title = `Lora${pagePrefix ? ` - ${pagePrefix}` : ''} - ${params.join(' - ')}`

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a small preference for omitting the Id part in transactions / assets / apps

So: Txn:${id} App:${id} Asset:${id}

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we omit the - separators, browser tabs would show more of the title before truncating:

Lora Txn ACSASDSADASD Mainnet vs Lora - Txn:ACSASDSADASD - Mainnet

May not make much difference though, so this is quite subjective

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with both of those suggestions.

Copy link
Author

Choose a reason for hiding this comment

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

Me too. I'll make these updates today.

pageTitle += ` - Group:${urlParams.groupId}`
}
if (urlParams?.address) {
pageTitle += ` - Addr:${urlParams.address}`
Copy link
Contributor

@tasosbit tasosbit Oct 30, 2024

Choose a reason for hiding this comment

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

The verbiage we use in these pages is Account (e.g. check <H1> there), so the document title should probably be Account or Acct instead of Addr to match that

Copy link
Author

Choose a reason for hiding this comment

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

👍 Will change to use Acct

@lempira
Copy link
Author

lempira commented Oct 31, 2024

I changed the page title format according to the suggested format. I also used @neilcampbell suggestion of joining the params into a string.

For the use-title test, I use a createMemoryBrowser of react-router-dom instead of mocking the useParams. I think this allows us to test more declaratively and pass in that URL instead of imperatively passing in the params.

There are many places that the params are mocked thru vi.mocked(useParams).mockImplementation. Perhaps we can create a helper wrapper that uses createMemoryBrowser and pass in the URL and not have to mock useParams?

Another suggestion: can we consider adding eslint-plugin-vitest to the testing conf. I had some focused tests that I forgot to unfocus, and they were only caught in the pipeline. The eslint plugin could have caught that, along with whatever other rules we want to enforce, on lint.

@neilcampbell
Copy link
Contributor

neilcampbell commented Oct 31, 2024

There are many places that the params are mocked thru vi.mocked(useParams).mockImplementation. Perhaps we can create a helper wrapper that uses createMemoryBrowser and pass in the URL and not have to mock useParams?

Another suggestion: can we consider adding eslint-plugin-vitest to the testing conf. I had some focused tests that I forgot to unfocus, and they were only caught in the pipeline. The eslint plugin could have caught that, along with whatever other rules we want to enforce, on lint.

Both of these sound good. @lempira Did you want to do these as part of this PR or handle separately?

const currentTitle = document.title
const pageTitleParams: string[] = []
if (urlParams.transactionId) {
pageTitleParams.push(`Txn:${urlParams.transactionId.slice(0, TRUNCATE_LENGTH)}`)
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
pageTitleParams.push(`Txn:${urlParams.transactionId.slice(0, TRUNCATE_LENGTH)}`)
pageTitleParams.push(`Txn:${ellipseId(urlParams.transactionId)}`)

This will inherit the same convention as we use elsewhere.

pageTitleParams.push(`Block:${urlParams.round}`)
}
if (urlParams?.groupId) {
pageTitleParams.push(`Group:${urlParams.groupId.slice(0, TRUNCATE_LENGTH)}`)
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
pageTitleParams.push(`Group:${urlParams.groupId.slice(0, TRUNCATE_LENGTH)}`)
pageTitleParams.push(`Group:${ellipseId(urlParams.groupId)}`)

pageTitleParams.push(`Group:${urlParams.groupId.slice(0, TRUNCATE_LENGTH)}`)
}
if (urlParams?.address) {
pageTitleParams.push(`Acct:${urlParams.address.slice(0, TRUNCATE_LENGTH)}`)
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
pageTitleParams.push(`Acct:${urlParams.address.slice(0, TRUNCATE_LENGTH)}`)
pageTitleParams.push(`Acct:${ellipseAddress(urlParams.address)}`)

import { useEffect } from 'react'
import { useParams } from 'react-router-dom'

const TRUNCATE_LENGTH = 10
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
const TRUNCATE_LENGTH = 10


describe('explore-page', () => {
describe('when no blocks are available', () => {
const myStore = createStore()

it('no latest blocks are displayed', () => {
vi.mocked(useParams).mockReturnValue({ round: '1234' })
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
vi.mocked(useParams).mockReturnValue({ round: '1234' })

AFAIK round isn't used as a param here. I also pushed a small change (as part of merging main into the branch) which removes the need for vi.mocked(useParams).mockReturnValue({}) in the tests that don't use params.

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.

3 participants