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

style: polish experiment detail [DET-3517, DET-3621] #974

Merged
merged 11 commits into from
Aug 5, 2020

Conversation

hkang1
Copy link
Contributor

@hkang1 hkang1 commented Jul 30, 2020

Description

Screen Shot 2020-07-30 at 9 11 57 AM

Screen Shot 2020-07-30 at 9 12 54 AM

Screen Shot 2020-07-30 at 9 14 42 AM

Test Plan

Commentary (optional)

@cla-bot cla-bot bot added the cla-signed label Jul 30, 2020
@hkang1 hkang1 force-pushed the 3621-polish-experiment-detail branch 3 times, most recently from db11c31 to b350776 Compare July 30, 2020 15:34
@hkang1 hkang1 changed the title style: polish experiment detail [DET-3621] style: polish experiment detail [DET-3517, DET-3621] Jul 30, 2020
@hkang1 hkang1 force-pushed the 3621-polish-experiment-detail branch from b350776 to 9b2aea1 Compare July 30, 2020 18:23
@hkang1 hkang1 requested a review from hamidzr August 3, 2020 15:04
@hkang1 hkang1 force-pushed the 3621-polish-experiment-detail branch from d7b3348 to a2552e7 Compare August 4, 2020 17:58
@hamidzr
Copy link
Member

hamidzr commented Aug 5, 2020

could we separate moving the files around from polishing and changing their content (in terms of PR)? the github UI doesn't give us a good diff of what's happening and what's changing

update: actually I could go commit by commit as you have them nice and separate.

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

The experiment page is looking so good now!

some style nits: I feel like we could use a higher contrast on the info labels.

Screen Capture_select-area_20200805120215

would be nice to have truncation on the experiment description and maybe a full tooltip? or maybe the full description could appear in infobox.

Screen Capture_select-area_20200805121030
Screen Capture_select-area_20200805121006

@@ -42,7 +42,7 @@ const TrialDetailsComp: React.FC = () => {
const message = isNotFound(trial.error) ? `Trial ${trialId} not found.`
: `Failed to fetch trial ${trialId}.`;
return (
<Page hideTitle title="Not Found">
<Page id="not-found">
Copy link
Member

Choose a reason for hiding this comment

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

nit: not a new addition here but not-found wouldn't be always accurate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the id to page-error-message to better reflect what the Page component is representing.

webui/react/src/pages/TaskList.tsx Show resolved Hide resolved
@@ -92,36 +90,42 @@ const ExperimentActions: React.FC<Props> = ({
};

const actionButtons: ConditionalButton<ExperimentDetails>[] = [
{ button: <Button key="fork" type="primary" onClick={onClick[Action.Fork]}>Fork</Button> },
{
Copy link
Member

Choose a reason for hiding this comment

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

sidenote: IMO if the buttons were defined separately we wouldn't have generated as big a diff with just reordering them

@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Aug 5, 2020
@hkang1 hkang1 force-pushed the 3621-polish-experiment-detail branch from b23ab8c to f728f38 Compare August 5, 2020 21:08
@hkang1 hkang1 merged commit 049a3c1 into determined-ai:master Aug 5, 2020
@hkang1 hkang1 deleted the 3621-polish-experiment-detail branch August 5, 2020 21:30
@dannysauer dannysauer added this to the 0.13.0 milestone Feb 6, 2024
eecsliu pushed 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