-
Notifications
You must be signed in to change notification settings - Fork 19
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 members to contributors & watchers in the side panel - UI only #3395
Split members to contributors & watchers in the side panel - UI only #3395
Conversation
letter-spacing: var(--spacing-medium); | ||
|
||
&:hover { | ||
background-color: color-mod(var(--action-secondary), alpha(12%)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comma is extra and is what causes the invalid color adjuster error.
isContributors, | ||
colony, | ||
currentDomainId = COLONY_TOTAL_BALANCE_DOMAIN_ID, | ||
maxAvatars = 12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe extract the constant out somewhere or set as a default since it is used twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the default value 12
should be a const
, but I like the approach of having the number of avatars configurable via props
src/modules/dashboard/components/ColonyHome/ColonyMembers/MembersSubsection.tsx
Show resolved
Hide resolved
src/modules/dashboard/components/ColonyHome/ColonyMembers/MembersSubsection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Left some optional notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good @chinins
The margin above the "Watchers" needs to be reduced by 10px
, as it has been done with the "Enabled extensions" heading, due to the margin under each avatar.
Also, it looks like you added in the carets, however, after consolidating the designs into the one spec, it was confirmed that the numbers was the correct approach, i.e. what was already in the DApp. This is my fault, so, my apologies for not knowing about the extra designs before handing it off.
See the spec here - https://www.figma.com/file/qQoBbyyDXkuHwoqHIOGjEu/Colony-Simplified?node-id=6736%3A91869
@chinins remove the the colon From this: background-color: color-mod(var(--action-secondary), alpha(12%)); To this background-color: color-mod(var(--action-secondary) alpha(12%)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, but this is some good work here! 💯
src/modules/dashboard/components/ColonyHome/ColonyMembers/MembersSubsection.tsx
Show resolved
Hide resolved
isContributors, | ||
colony, | ||
currentDomainId = COLONY_TOTAL_BALANCE_DOMAIN_ID, | ||
maxAvatars = 12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the default value 12
should be a const
, but I like the approach of having the number of avatars configurable via props
src/modules/dashboard/components/ColonyHome/ColonyMembers/MembersSubsection.tsx
Outdated
Show resolved
Hide resolved
src/modules/dashboard/components/ColonyHome/ColonyMembers/ColonyMembers.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I didn't interpret it that way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! I think the others already pointed out the things that perhaps need a change so I'm just going to approve as everything else looks good to me
ac17825
to
0401163
Compare
@arrenv, I've updated it. Please, have a look and give your blessing for me to merge :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, nice one!
Description
This PR splits members in the side panel into contributors & watchers.
New stuff ✨
MembersSubsection
file to generalise codeTO DO
I have a warning in my terminal (screen shot below) and I don't understand the reason for it. I know that it's coming from this line (line 57 in ColonyMembers.css):
background-color: color-mod(var(--action-secondary), alpha(12%));
But I use it exactly the same as in other places. I'll keep investigating but I ran out of ideas for now. Maybe somebody knows why I have the warning?
Resolves #3378