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

Add icon to page header if an account is a contract #348 #350

Merged
merged 1 commit into from
Feb 16, 2025

Conversation

duyluong1994
Copy link
Contributor

Resolve #348

@aaroncox aaroncox linked an issue Feb 2, 2025 that may be closed by this pull request
@aaroncox aaroncox changed the base branch from api-v2 to dev February 4, 2025 00:10
Copy link
Contributor

@deansallinen deansallinen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A couple of small changes can be made, but otherwise it looks good.

Comment on lines +30 to +38
<button
onclick={goToContract}
class={cn(
'peer absolute left-1/2 top-1/2 -translate-x-1/2 -translate-y-1/2 focus-visible:outline-none',
buttonSize
)}
>
<!-- Button is done this way with absolute positioning so we can maintain a decent hit slop on mobile without affecting layout -->
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Navigation should be done with a link (anchor tag) when possible to take advantage of preloading

Comment on lines +30 to +38
<button
onclick={goToContract}
class={cn(
'peer absolute left-1/2 top-1/2 -translate-x-1/2 -translate-y-1/2 focus-visible:outline-none',
buttonSize
)}
>
<!-- Button is done this way with absolute positioning so we can maintain a decent hit slop on mobile without affecting layout -->
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be a link

Comment on lines 60 to 68
{#if routePath === 'account'}
<CopyButton data={props.title} />
<CopyButton data={props.title} slop={false} />
{#if props.contract}
<CodeButton data={contractPath} slop={false} />
{/if}
{/if}
{#if routePath === 'contract'}
<AccountButton data={accountPath} slop={false} />
{/if}
Copy link
Contributor

Choose a reason for hiding this comment

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

With this layout, the button under the cursor changes depending on the page - we should keep consistency and either add a copy button to the contract page, or swap the order so the switcher button is first.

@aaroncox
Copy link
Member

Merged. Opened issue #372 to track the requested changes.

@aaroncox aaroncox merged commit 3d0844c into greymass:dev Feb 16, 2025
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.

Add icon to page header if an account is a contract
3 participants