refactor: replace AvatarIcon instances with FacePile #11279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11279 +/- ##
==========================================
+ Coverage 65.43% 65.63% +0.20%
==========================================
Files 831 837 +6
Lines 39528 39685 +157
Branches 3598 3608 +10
==========================================
+ Hits 25866 26049 +183
+ Misses 13553 13528 -25
+ Partials 109 108 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/components/UserStack/UserStack.stories.tsx
Outdated
Show resolved
Hide resolved
| {users.map(({ first_name, last_name }) => { | ||
| const name = `${first_name} ${last_name}`; | ||
| return ( | ||
| <Tooltip key={name} title={name} placement="top"> |
There was a problem hiding this comment.
this key won't be unique if the same user is an owner of multiple dashboards i think. Perhaps we need to append more context here?
There was a problem hiding this comment.
this is rendered in the context of a single row for dashboards. The array here is for owers on a single dashboard and that list should be unique. While the key would not be unique for the whole page, it would be unique for the list in which each appears.
| */ | ||
|
|
||
| // https://en.wikipedia.org/wiki/Linear_congruential_generator | ||
| function stringAsciiPRNG(value: string, m: number) { |
There was a problem hiding this comment.
can we add a test or two for this?
There was a problem hiding this comment.
Also, what if they have a non english/ascii name?
There was a problem hiding this comment.
This should be covered in the tests for getRandomColor
|
NIT: tooltip-in-popover is a bit "yo dawg" :) |
Yea it's a little much visually. We could remove it (ie, check to see if the avatar is beyond the fold and not add the tooltip) but then we'd lose some information. |
nytai
changed the title
| import FacePile from '.'; | ||
|
|
||
| export default { | ||
| title: 'UerStack', |
There was a problem hiding this comment.
nit: this should probably be FacePile for consistency
| <StyledGroup maxCount={maxCount}> | ||
| {users.map(({ first_name, last_name }) => { | ||
| const name = `${first_name} ${last_name}`; | ||
| const color = getRandomColor(name, colorList); |
There was a problem hiding this comment.
this doesn't need to be addressed in this PR, but basing the profile color off the full name seems somewhat flawed to me. If there are naming collisions between people with common full names, then it'll be impossible to tell them apart in the FacePile. This could also cause key collisions below.
I'd recommend refactoring in the future to basing the color off the user id, or a combo of name + user id.
There was a problem hiding this comment.
That's a good point, I can add user id to the mix and that should hold us over for a while. Eventually, I think, we'll have profile pictures and make these links so it's easier to differentiate users
mistercrunch
added
🏷️ bot
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION