Skip to content
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

feat: add archived status column to Experiment list [DET-3540] #967

Conversation

armandmcqueen
Copy link
Contributor

@armandmcqueen armandmcqueen commented Jul 29, 2020

Description

Add column to Experiment list that uses a checkmark to indicate whether the Experiment is archived.

Test Plan

Tested manually.

Screen Shot 2020-07-29 at 4 57 33 AM

Tested sorting - when using descending order, archived=True is at the top.

Screen Shot 2020-07-29 at 4 57 48 AM

Commentary (optional)

I found a checkmark in Ant Design that I think looks good, but let me know if we want to go with Yes/No text for now.

Comment on lines 110 to 111
if (!record.archived) return null;
return <CheckOutlined />;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: this can use the ternary operator:

return record.archived ? <CheckOutlined /> : null;

@@ -75,7 +76,7 @@ export const taskTypeRenderer: TaskRenderer = (_, record) => (
</Tooltip>
);

/* Experiemnt Table Column Renderers */
/* Experiment Table Column Renderers */
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 english is hard :)

align: 'center',
render: experimentArchivedRenderer,
sorter: (a: ExperimentItem, b: ExperimentItem): number =>
(a.archived === b.archived)? 0 : a.archived? 1 : -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: nice, love the nested ternary. Don't have to this now but if we see another boolean based sorting use case, we can abstract into a dedicated booleanSorter. How about this for a little better legibility?

(a.archived === b.archived) ? 0 : (a.archived ? 1 : -1),

Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

Nice work with this PR! 🎉 Some of suggestions and nits, but nothing blocking.

@hkang1 hkang1 assigned armandmcqueen and unassigned hkang1 Jul 29, 2020
@armandmcqueen armandmcqueen force-pushed the add_archived_col_to_experiment_list branch from e1f8906 to 9dab7f9 Compare July 30, 2020 17:49
@armandmcqueen armandmcqueen force-pushed the add_archived_col_to_experiment_list branch from e7f19e0 to bb4d8cd Compare July 31, 2020 09:07
@armandmcqueen armandmcqueen merged commit 902a3e6 into determined-ai:master Aug 3, 2020
stoksc pushed a commit that referenced this pull request Oct 17, 2023
azhou-determined pushed a commit that referenced this pull request Dec 7, 2023
wes-turner pushed a commit that referenced this pull request Feb 2, 2024
@dannysauer dannysauer added this to the 0.12.13 milestone Feb 6, 2024
amandavialva01 pushed a commit that referenced this pull request Mar 18, 2024
eecsliu added a commit that referenced this pull request Apr 18, 2024
eecsliu added a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants