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

feat: Keyblock microblocks table #382

Merged
merged 24 commits into from
Jul 24, 2023
Merged

Conversation

janmichek
Copy link
Collaborator

@janmichek janmichek commented Jul 18, 2023

Description

resolves #369

Demo

firefox_ViAWRf1ZeL.mp4

Checklist:

@github-actions
Copy link

Deployed to https://pr-382-aescan.stg.aepps.com

@janmichek janmichek requested review from lukeromanowicz and michele-franchi and removed request for lukeromanowicz July 19, 2023 13:12
Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Good job, I have some comments.

Any reason why the hash is not truncated? It would be the only table like this.
image

It's not clear that in general these are microblocks, it is not written anywhere.

v-for="microblock in microblocks.data"
:key="microblock.hash">
<td>
<app-link :to="`/micro-blocks/${microblock.hash}`">
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
<app-link :to="`/micro-blocks/${microblock.hash}`">
<app-link :to="`/microblocks/${microblock.hash}`">

The link is incorrect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@janmichek
Copy link
Collaborator Author

@michele-franchi

Any reason why the hash is not truncated? It would be the only table like this.

I was following the visual material. No strong preference here

It's not clear that in general these are microblocks, it is not written anywhere.

I was also following the visual material. Here I would prefer label as well.

What do you think about those changes @danilosierrac ?

Copy link
Contributor

@lukeromanowicz lukeromanowicz left a comment

Choose a reason for hiding this comment

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

Looks really good. Thank you for implementing it! I left one small note but I approve in advance

const { fetchKeyblockMicroblocks } = useKeyblockDetailsStore()
const route = useRoute()
const limit = computed(() => process.client && isDesktop() ? 10 : 3)
fetchKeyblockMicroblocks({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be fetched on the client side only. Right now it's fetched also on server-side but the SSR doesn't wait for the response so you don't see any warnings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Please have a look at the recent changes I suggested in the design:

  • A new tab is visible called "Microblocks"
    I would still use truncated hashes for consistency but I don't have a strong preference.

@janmichek
Copy link
Collaborator Author

  • A new tab is visible called "Microblocks"

Ok, both adjusted

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Good job, thanks for the fixes 👏

@janmichek janmichek merged commit 950aa3b into develop Jul 24, 2023
@janmichek janmichek deleted the Keyblock_microblocks_table branch July 24, 2023 09:30
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.

Keyblock microblocks table
4 participants