-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Dashboard] Add "Creator" column to dashboard listing page #182256
Conversation
/ci |
/ci |
/ci |
Pinging @elastic/kibana-presentation (Team:Presentation) |
Pinging @elastic/appex-sharedux (Team:SharedUX) |
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.
Not sure if there is an easier way to test the "No user" situation (imported dashboards get metadata injected, so even manually removing it won't work) - but I ended up checking out a PR from before #179344 was merged and ran that in order to create the "No user" dashboards. (Please let me know if there is an easier way 🙈)
In this test, I noticed that the "Created by" column only gets filtered out on hard refresh:
Screen.Recording.2024-05-02.at.9.51.06.AM.mov
This isn't the best UX, but also not sure how common of an experience this will be?
I prefer to not block the dashboard rendering with the additional getAllUsers request to make the column sortable.
💯 I think performance of the listing page is much more important than sorting. That being said, we could still render the listing page and show a loading state for just the creator column, no? And, assuming if we resolve what I noted above, this column could still be hidden if, after the request is completed, none of the dashboards have created-by metadata.
That being said, none of the things I've mentioned here (or in the attached comments) are blockers - so I'm approving :) So excited to see this work! 🎉
return ( | ||
<UserAvatarTipComponent | ||
user={query.data.user} | ||
avatar={query.data.data.avatar} | ||
size={'s'} | ||
data-test-subj={`userAvatarTip-${query.data.user.username}`} | ||
/> | ||
); |
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.
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.
Haha, thanks :) I fixed the popover but didn't notice the toolip
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.
Good find @Heenawter ! @Dosant should I create an issue for any of our teams to handle that or are you planning to fix it in this PR?
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.
I'll create an issue
test("shouldn't render created by column when createdBy is disabled", async () => { | ||
render(<TableListView {...requiredProps} />); | ||
|
||
// wait until first render | ||
expect(await screen.findByTestId('itemsInMemTable')).toBeVisible(); | ||
|
||
expect(() => screen.getByTestId(/tableHeaderCell_createdBy/)).toThrow(); | ||
}); |
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.
Nice 🎉 Good to have this test since it's not an easy situation to encounter.
const query = useQuery( | ||
['user-profile', props.uid], | ||
async () => { | ||
return getUserProfile(props.uid); | ||
}, | ||
{ staleTime: Infinity } | ||
); |
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.
Because we are only using the uid
as our query key here, it looks like the custom avatars don't get refetched until after a hard refresh:
Screen.Recording.2024-05-02.at.9.13.37.AM.mov
I don't consider this to be a blocker by any means, because I don't think it's critical that this gets updated right away necessarily - especially if this would impact performance. But figured it was worth noting.
It seems that this logic was written for "last_updated_at" column and I think it was intentional to prevent unwanted table shift layouts. I think this fine. But your screenshot made me want to add a question icon tip next to "Creator" column label explaining that only new dashboards get creator. |
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.
LGTM! Tested locally and works as expected 👍
We should be able to refetch when component mounts with react query |
@elasticmachine merge upstream |
…a into d/2024-05-01-creator-column
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @Dosant |
Summary
close #182253
This PR adds a creator column to the dashboard's listing page: