Skip to content

Conversation

nikkikapadia
Copy link
Member

As part of creating table view for the dashboards landing page this PR creates the actual table with most of the fields that are present in the mockup (only column left to add is permissions/access column). Switching the toggle to list view will bring you to this page view. I've defaulted to 25 items per page (i'm not sure if there's a different amount we want to use). It's still under the feature flag dashboards-table-view which I only have access to atm.

image

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 18, 2024
@nikkikapadia nikkikapadia marked this pull request as ready for review November 18, 2024 21:49
@nikkikapadia nikkikapadia requested a review from a team as a code owner November 18, 2024 21:49
import {type DashboardListItem, DisplayType} from 'sentry/views/dashboards/types';

describe('Dashboards - DashboardList', function () {
describe('Dashboards - DashboardTable', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('Dashboards - DashboardTable', function () {
describe('Dashboards - DashboardGrid', function () {

/>
);

expect(screen.getByTestId('empty-state')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assert on the text so we can validate what the user behaviour?

isLoading?: boolean;
};

enum Keys {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum Keys {
enum ResponseKeys {

small nit, but just thinking that Keys is pretty generic, if we're going to make an enum we should make it clear what keys it's meant to be for

function handleDelete(dashboard: DashboardListItem) {
deleteDashboard(api, organization.slug, dashboard.id)
.then(() => {
trackAnalytics('dashboards_manage.delete', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important for now, but it may be interesting to add the view type (table, grid) to the event in case we see different behaviour on either page for actions like delete, duplicate, etc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo ya good point. I'll make a ticket for that so I remember before we rollout 😄

return <span>{dataRow[column.key]}</span>;
};

function renderDashboardTable() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can unnest this function's body and it'll render the dashboard table

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 90.56604% with 5 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...tic/app/views/dashboards/manage/dashboardTable.tsx 91.66% 4 Missing ⚠️
static/app/views/dashboards/manage/index.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #80945   +/-   ##
=======================================
  Coverage   78.42%   78.43%           
=======================================
  Files        7211     7215    +4     
  Lines      319738   319731    -7     
  Branches    44008    44023   +15     
=======================================
+ Hits       250749   250771   +22     
+ Misses      62602    62573   -29     
  Partials     6387     6387           

@nikkikapadia nikkikapadia merged commit f32b2d2 into master Nov 20, 2024
44 checks passed
@nikkikapadia nikkikapadia deleted the nikki/feat/make-dashboards-table branch November 20, 2024 19:13
harshithadurai pushed a commit that referenced this pull request Nov 25, 2024
As part of creating table view for the dashboards landing page this PR
creates the actual table with most of the fields that are present in the
mockup (only column left to add is permissions/access column). Switching
the toggle to list view will bring you to this page view. I've defaulted
to 25 items per page (i'm not sure if there's a different amount we want
to use). It's still under the feature flag `dashboards-table-view` which
I only have access to atm.

<img width="1512" alt="image"
src="https://github.com/user-attachments/assets/89530817-cd94-4128-ad3a-fa1ea05851d6">
evanh pushed a commit that referenced this pull request Nov 25, 2024
As part of creating table view for the dashboards landing page this PR
creates the actual table with most of the fields that are present in the
mockup (only column left to add is permissions/access column). Switching
the toggle to list view will bring you to this page view. I've defaulted
to 25 items per page (i'm not sure if there's a different amount we want
to use). It's still under the feature flag `dashboards-table-view` which
I only have access to atm.

<img width="1512" alt="image"
src="https://github.com/user-attachments/assets/89530817-cd94-4128-ad3a-fa1ea05851d6">
andrewshie-sentry pushed a commit that referenced this pull request Dec 2, 2024
As part of creating table view for the dashboards landing page this PR
creates the actual table with most of the fields that are present in the
mockup (only column left to add is permissions/access column). Switching
the toggle to list view will bring you to this page view. I've defaulted
to 25 items per page (i'm not sure if there's a different amount we want
to use). It's still under the feature flag `dashboards-table-view` which
I only have access to atm.

<img width="1512" alt="image"
src="https://github.com/user-attachments/assets/89530817-cd94-4128-ad3a-fa1ea05851d6">
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants