feat: owners profile icon on dataset list view #10041

lilykuang · 2020-06-11T19:26:55Z

SUMMARY

This profile icon is part of the dataset redesign SIP-34.

Add a new react component react-avatar
Add owners column to dataset list view
Implement circle icon with owner's initials as owners' profile icon
owners column displays maximum of 5 icons
When :hover, tooltip should contain the owner's name

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

updated color scheme:

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter · 2020-06-11T19:32:56Z

Codecov Report

Merging #10041 into master will increase coverage by 1.61%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master   #10041      +/-   ##
==========================================
+ Coverage   68.88%   70.49%   +1.61%     
==========================================
  Files         584      585       +1     
  Lines       31058    31074      +16     
  Branches     3180     3185       +5     
==========================================
+ Hits        21393    21905     +512     
+ Misses       9555     9060     -495     
+ Partials      110      109       -1

Flag	Coverage Δ
#cypress	`53.90% <ø> (+0.07%)`	⬆️
#javascript	`59.49% <61.11%> (+<0.01%)`	⬆️
#python	`70.07% <ø> (+2.72%)`	⬆️

Impacted Files	Coverage Δ
superset-frontend/src/components/AvatarIcon.tsx	`58.33% <58.33%> (ø)`
...set-frontend/src/views/datasetList/DatasetList.tsx	`56.14% <66.66%> (+0.58%)`	⬆️
.../src/dashboard/components/gridComponents/Chart.jsx	`88.76% <0.00%> (-1.13%)`	⬇️
superset/views/dashboard/filters.py	`95.23% <0.00%> (-0.22%)`	⬇️
superset/utils/core.py	`89.03% <0.00%> (-0.16%)`	⬇️
superset/dashboards/filters.py	`96.55% <0.00%> (-0.12%)`	⬇️
superset/security/manager.py	`89.04% <0.00%> (-0.08%)`	⬇️
superset/views/database/forms.py	`90.69% <0.00%> (ø)`
superset/views/database/decorators.py	`96.15% <0.00%> (ø)`
...et-frontend/src/dashboard/components/Dashboard.jsx	`84.33% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54c6ddb...a5d5ce0. Read the comment docs.

etr2460

a few comments, looks like a cool new feature!

etr2460 · 2020-06-11T22:32:50Z

superset-frontend/src/components/AvatarIcon.tsx

+    const { tableName, firstName, lastName, userName, iconSize } = this.props;
+    const uniqueKey = tableName.concat('_', userName);
+    const fullName = firstName.concat(' ', lastName);
+    const colors = [


can we pull this array out into a constant so that it doesn't get rebuilt on every render?

Curious where those colors came from. We should use the main palette (https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-color/src/colorSchemes/categorical/airbnb.ts) for now and eventually the pretty SIP-34 ones.

etr2460 · 2020-06-11T22:36:05Z

superset-frontend/src/components/AvatarIcon.tsx

+            name={fullName}
+            size={iconSize}
+            round
+            style={{ margin: '0px 5px' }}


can we use the className attribute and apply a class to this instead of inlining styles?

Since we may want consistent avatar styles, any additional CSS will likely need to be set globally. If margin is the only style we will set for the avatar (which probably can be changed to either a variable or proportional to iconSize), I think it's OK to use inline style.

Actually, looking at this closer, it seems like this styling should be on a wrapper around the avatar, perhaps an AvatarGroup component? Since it seems like this styling is only needed when displaying multiple avatars next to each other

or wrap it with emotion styled

etr2460 · 2020-06-11T22:37:58Z

superset-frontend/src/components/AvatarIcon.tsx

+
+interface Props {
+  firstName: string;
+  iconSize: string;


can this be a number instead? Seems a bit odd as a string and react-avatar seems to accept either a number of a string

@etr2460 I have tried using a number but size of react-avatar could only accept string

etr2460 · 2020-06-11T22:38:24Z

superset-frontend/src/components/AvatarIcon.tsx

+  userName: string;
+}
+
+export default class AvatarIcon extends PureComponent<Props> {


Since this component only has a render function, maybe we should make it a functional component instead?

etr2460 · 2020-06-11T22:39:40Z

superset-frontend/src/components/AvatarIcon.tsx

+export default class AvatarIcon extends PureComponent<Props> {
+  render() {
+    const { tableName, firstName, lastName, userName, iconSize } = this.props;
+    const uniqueKey = tableName.concat('_', userName);


since unique key is only used in one place, instead of defining as a constant it might be cleaner to set it directly on the key attribute

@etr2460 uniqueKey is also used as key for <Avatar> https://github.com/apache/incubator-superset/blob/43e81a0f9d6e20d95a33f4a2f98664505a32c3f8/superset-frontend/src/components/AvatarIcon.tsx#L51

etr2460 · 2020-06-11T22:39:54Z

superset-frontend/src/components/AvatarIcon.tsx

+export default class AvatarIcon extends PureComponent<Props> {
+  render() {
+    const { tableName, firstName, lastName, userName, iconSize } = this.props;
+    const uniqueKey = tableName.concat('_', userName);


nit: prefer ${tableName}_${userName}

etr2460 · 2020-06-11T22:40:11Z

superset-frontend/src/components/AvatarIcon.tsx

+  render() {
+    const { tableName, firstName, lastName, userName, iconSize } = this.props;
+    const uniqueKey = tableName.concat('_', userName);
+    const fullName = firstName.concat(' ', lastName);


nit: prefer ${firstName} ${lastName}

etr2460 · 2020-06-11T23:32:21Z

superset-frontend/package.json

@@ -140,6 +140,7 @@
    "re-resizable": "^4.3.1",
    "react": "^16.13.0",
    "react-ace": "^5.10.0",
+    "react-avatar": "^3.9.7",


Is this something that we might want to migrate away from in the future if the Antd discussions we were talking about in the meetup today come to fruition? I see that antd has an avatar component already: https://ant.design/components/avatar/

Possibly, but we can't block forward progress based on a pre-SIP discussion for something that may or may not pass a vote. Should AntD come to fruition, we can swap out components later.

I'm not saying to block progress, moreso wondering if since we're adding a new library anyway if we should think ahead. Either way, whichever you think is best works

etr2460

a couple other nits, but approving to unblock since i'm going to be away from my laptop for most of today

etr2460 · 2020-06-12T17:09:44Z

superset-frontend/package.json

@@ -140,6 +140,7 @@
    "re-resizable": "^4.3.1",
    "react": "^16.13.0",
    "react-ace": "^5.10.0",
+    "react-avatar": "^3.9.7",


I'm not saying to block progress, moreso wondering if since we're adding a new library anyway if we should think ahead. Either way, whichever you think is best works

etr2460 · 2020-06-12T17:11:27Z

superset-frontend/src/components/AvatarIcon.tsx

+  const fullName = `${firstName} ${lastName}`;
+
+  return (
+    <ConfigProvider colors={colorList && colorList.colors}>


what happens if this is undefined? Does react-avatar have a sane fallback?

react-avatar has fallback colors

etr2460 · 2020-06-12T17:11:59Z

superset-frontend/src/components/AvatarIcon.tsx

+        overlay={<Tooltip id={`${uniqueKey}-tooltip`}>{fullName}</Tooltip>}
+      >
+        <StyledAvatar
+          key={`${uniqueKey}`}


this can be key={uniqueKey}

etr2460 · 2020-06-12T17:12:11Z

superset-frontend/src/components/AvatarIcon.tsx

+        <StyledAvatar
+          key={`${uniqueKey}`}
+          name={fullName}
+          size={`${iconSize}`}


this can be size={iconSize}

etr2460 · 2020-06-12T17:13:45Z

superset-frontend/src/views/datasetList/DatasetList.tsx

@@ -54,13 +62,14 @@ interface State {
 }

 interface Dataset {
+  changed_by: string;


thanks for the abc!

add avatar icon to dataset list view

43e81a0

superset-github-bot bot added the preset-io label Jun 11, 2020

pull-request-size bot added the size/M label Jun 11, 2020

lilykuang force-pushed the lily/datasets/owner-profile-icon branch from fcfef3f to 43e81a0 Compare June 11, 2020 19:32

etr2460 reviewed Jun 11, 2020

View reviewed changes

update avatar icon component

56f4290

pull-request-size bot added size/L and removed size/M labels Jun 12, 2020

use color scheme from superset-ui

edf8a09

lilykuang requested review from etr2460 and nytai June 12, 2020 17:07

etr2460 approved these changes Jun 12, 2020

View reviewed changes

update avatar unique key

a5d5ce0

pull-request-size bot added size/M and removed size/L labels Jun 12, 2020

nytai merged commit 98ab95e into apache:master Jun 15, 2020

nytai deleted the lily/datasets/owner-profile-icon branch June 15, 2020 16:52

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

feat: owners profile icon on dataset list view (apache#10041)

f1a2b39

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: owners profile icon on dataset list view #10041

feat: owners profile icon on dataset list view #10041

lilykuang commented Jun 11, 2020 •

edited

codecov-commenter commented Jun 11, 2020 •

edited

etr2460 left a comment

etr2460 Jun 11, 2020

mistercrunch Jun 12, 2020

etr2460 Jun 11, 2020

ktmud Jun 11, 2020

etr2460 Jun 11, 2020

nytai Jun 11, 2020

etr2460 Jun 11, 2020

lilykuang Jun 12, 2020

etr2460 Jun 11, 2020

etr2460 Jun 11, 2020

lilykuang Jun 12, 2020

etr2460 Jun 11, 2020

etr2460 Jun 11, 2020

etr2460 Jun 11, 2020

willbarrett Jun 12, 2020

etr2460 Jun 12, 2020

etr2460 left a comment

etr2460 Jun 12, 2020

etr2460 Jun 12, 2020

lilykuang Jun 12, 2020

etr2460 Jun 12, 2020

etr2460 Jun 12, 2020

etr2460 Jun 12, 2020

feat: owners profile icon on dataset list view #10041

feat: owners profile icon on dataset list view #10041

Conversation

lilykuang commented Jun 11, 2020 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter commented Jun 11, 2020 • edited

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilykuang commented Jun 11, 2020 •

edited

codecov-commenter commented Jun 11, 2020 •

edited