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

Split registrars display #6157

Merged
merged 57 commits into from
Sep 29, 2021
Merged

Split registrars display #6157

merged 57 commits into from
Sep 29, 2021

Conversation

MiZiet
Copy link
Collaborator

@MiZiet MiZiet commented Sep 20, 2021

If applied these changes will split displayed judgments (if those differ) and show registrars in popups:

image

@krzysztof-jelski
Copy link
Collaborator

It doesn't work when user has no accounts (the popup shows empty in this case). It's due to the hasAccounts condition here: https://github.com/polkadot-js/apps/blob/master/packages/react-hooks/src/useRegistrars.ts#L32.
I think hasAccounts should just prevent the check for "is any of my accounts a registrar".

@MiZiet
Copy link
Collaborator Author

MiZiet commented Sep 21, 2021

hi, @krzysztof-jelski we've moved hasAccount check to the isRegistrar and extracted useJudgements hook.

@MiZiet MiZiet requested a review from jacogr September 21, 2021 14:29
Copy link
Collaborator

@ekowalsk ekowalsk left a comment

Choose a reason for hiding this comment

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

Looks great!

@MiZiet
Copy link
Collaborator Author

MiZiet commented Sep 24, 2021

hi @jacogr, updated with master and ready for review :)

const judgementColor = useMemo(() => getJudgementColor(judgementName), [judgementName]);

return (
<Popup
Copy link
Member

Choose a reason for hiding this comment

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

This is very weird - I can see the code, I can see your screenshot, but I don't get this info on my screen at all. Hover over the "Reasonable" and it is just, well, a cursor, no popup :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some rebuild perhaps? With yarn clean and yarn build?

Copy link
Collaborator Author

@MiZiet MiZiet Sep 24, 2021

Choose a reason for hiding this comment

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

The popup shows on click not on hover, did you try to click it?

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh... yes, not hover, click. (Not quite obvious to me :))

color='yellow'
isTag={false}
key='NoJudgements'
label='No Judgements'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
label='No Judgements'
label={t<string>('No Judgements')}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 69 to 71
<Judgements
address={address}
/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Judgements
address={address}
/>
<Judgements address={address} />

Weird, could have sworn the linter would pick this up (single attribute, multiple lines)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jacogr
Copy link
Member

jacogr commented Sep 24, 2021

Happy, this is a nice addition. However some tests are failing.

@ekowalsk
Copy link
Collaborator

Tests fixed ✅

@krzysztof-jelski
Copy link
Collaborator

@jacogr tests are fixed now, are you ok with us merging it?

@jacogr
Copy link
Member

jacogr commented Sep 29, 2021

All happy.

@jacogr jacogr merged commit 053e0ef into master Sep 29, 2021
@jacogr jacogr deleted the ek-mz-registars branch September 29, 2021 08:45
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Differentiate between registrars in account sidebar
5 participants