Show colored UI avatars for new users#396
Conversation
made changes to member ui avataar
ivinayakg
left a comment
ivinayakg
left a comment
There was a problem hiding this comment.
gave some suggestions, about the code which I belive can help make the code better
| developerInfo={newMember} | ||
| isMember={false} | ||
| isOptionKey={isOptionKey} | ||
| colorCombination={cardColorArray[prev]} |
There was a problem hiding this comment.
instead of prev which is a non-reactive state, use a random function that generates the random no. between a range and pass that
Math.floor(Math.random() * (cardColorArray.length))
There was a problem hiding this comment.
I would have done that In the first place, but since we decided on some kind of a pattern to be used I passed the colors in a sequence. But i dont get why non-reactive state is important here. We are not re-rendering the cards again once they are loaded.
There was a problem hiding this comment.
It's a good practice in my opinion to be safe from the edge cases
There was a problem hiding this comment.
can you elaborate more on this , help me get a better context
There was a problem hiding this comment.
I also want to remind @vickydonor-99, we want some sort of hashing function, that distributes the colors, but makes it such that the same color is always applied to the same username.
There was a problem hiding this comment.
yes @ankushdharkar, haven't got a clear idea about it yet
| { color_primary: '#165692', color_secondary: '#3D8BD3' }, | ||
| { color_primary: '#264653', color_secondary: '#387892' }, | ||
| ]; | ||
| let counter = 0; |
There was a problem hiding this comment.
Why it is exposed to global scope?
|
And if we look into screenshot colors are dark, can we make us of subtle colors? |
ankushdharkar
left a comment
ankushdharkar
left a comment
There was a problem hiding this comment.
Why has the package-lock updated?
The package-lock should not be changed, unless:
- The package.json is updated
- The PR is doing maintenance/upkeep of codebase
| import SuperUserOptions from '@components/member-card/super-user-options'; | ||
|
|
||
| const Card = ({ developerInfo, isOptionKey }) => { | ||
| const Card = ({ developerInfo, isOptionKey, colorCombination }) => { |
There was a problem hiding this comment.
I am a little confused. We are updating the cards for the users. Why are we adding another prop in member-card for it?
There was a problem hiding this comment.
logic for member and new-member was present in the same card to begin with
| cardElement.style.border = `2px solid ${color_primary}`; | ||
| cardElement.style.backgroundColor = color_secondary; | ||
| textElement.innerText = `${first_name[0].toUpperCase()} ${last_name[0].toUpperCase()}`; |
There was a problem hiding this comment.
Can the styling come from CSS modifiers instead?
We add the modifying class for the color(s), and the CSS included does the rest? Will make our code a lot more declarative in nature, and reduce the number of lines here.
There was a problem hiding this comment.
for now i have added inline styles, later might make the suggested change, once I get the idea about the hashing function
ankushdharkar
changed the title
made changes requested in the PR
I have not updated the |
@vickydonor-99 |
| color_primary: '#DB1212', | ||
| color_secondary: '#F88181', |
There was a problem hiding this comment.
can we move these constants to a constat file and then import them here and use it? In the future, we might need these default colors elsewhere also. What do you think?
added hash function for consistent colors for members
installed deps acc to pakacge.json
6 participants
What is the change?
Changed ui-avatars of new members, . issue 382
Is it bug? No
*Dev Tested?
*Tested on:
browsers
Platforms
Browsers
Before / After Change Screenshots
Before
##After
updated (after)