Skip to content

split out pool detailed info into a standalone settings page#26805

Merged
prha merged 2 commits intomasterfrom
prha/ui_instance_page
Feb 3, 2025
Merged

split out pool detailed info into a standalone settings page#26805
prha merged 2 commits intomasterfrom
prha/ui_instance_page

Conversation

@prha
Copy link
Copy Markdown
Member

@prha prha commented Jan 3, 2025

Summary & Motivation

With #26804, we're directing more attention to the settings for specific pools.

This PR creates a new dedicated view for specific pool information (was previously in a dialog).

Screen.Recording.2025-01-23.at.11.16.45.AM.mov

How I Tested These Changes

BK

Changelog

Creates a standalone view for specific pool info

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2025

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-o2bou273d-elementl.vercel.app
https://prha-ui-instance-page.core-storybook.dagster-docs.io

Built with commit bec08b1.
This pull request is being automatically deployed with vercel-action

@prha prha force-pushed the prha/ui_pool_reference branch from 18c1cfc to c01da8d Compare January 6, 2025 20:04
@prha prha force-pushed the prha/ui_instance_page branch from c5aaa65 to d269905 Compare January 6, 2025 20:04
@prha prha force-pushed the prha/ui_pool_reference branch from c01da8d to 15bc61b Compare January 6, 2025 20:52
@prha prha force-pushed the prha/ui_instance_page branch from d269905 to 05cbe94 Compare January 6, 2025 20:52
@prha prha force-pushed the prha/ui_pool_reference branch from 15bc61b to 81fd01b Compare January 6, 2025 22:06
@prha prha force-pushed the prha/ui_instance_page branch from 05cbe94 to f508458 Compare January 6, 2025 22:06
@prha prha force-pushed the prha/ui_pool_reference branch from 81fd01b to 948ce8f Compare January 7, 2025 17:17
@prha prha force-pushed the prha/ui_instance_page branch from f508458 to 36ef1c1 Compare January 7, 2025 17:17
@prha prha force-pushed the prha/ui_pool_reference branch from 948ce8f to e95b257 Compare January 7, 2025 18:54
@prha prha force-pushed the prha/ui_instance_page branch from 36ef1c1 to 4eb991b Compare January 7, 2025 18:54
@prha prha force-pushed the prha/ui_pool_reference branch from e95b257 to 02311b6 Compare January 7, 2025 22:53
@prha prha force-pushed the prha/ui_instance_page branch from 4eb991b to b50b563 Compare January 7, 2025 22:54
@prha prha force-pushed the prha/ui_pool_reference branch from 02311b6 to fd552b6 Compare January 8, 2025 04:25
@prha prha force-pushed the prha/ui_instance_page branch from b50b563 to 53edb22 Compare January 8, 2025 04:25
@prha prha force-pushed the prha/ui_pool_reference branch from fd552b6 to f1ee75f Compare January 8, 2025 04:49
@prha prha force-pushed the prha/ui_instance_page branch 2 times, most recently from 87c0a6f to d24cf46 Compare January 8, 2025 04:59
@prha prha force-pushed the prha/ui_pool_reference branch from f1ee75f to 3c5deda Compare January 8, 2025 05:13
@prha prha force-pushed the prha/ui_instance_page branch from d24cf46 to 80d3b13 Compare January 8, 2025 05:13
@prha prha force-pushed the prha/ui_pool_reference branch from 3c5deda to 9fa4539 Compare January 8, 2025 05:17
@prha prha force-pushed the prha/ui_instance_page branch 2 times, most recently from c3e9fa3 to 7d4debd Compare January 8, 2025 05:26
@prha prha force-pushed the prha/ui_pool_reference branch from 9fa4539 to 16f3f44 Compare January 8, 2025 19:52
@prha prha force-pushed the prha/ui_instance_page branch from 7d4debd to 7903c5f Compare January 8, 2025 19:52
@prha prha force-pushed the prha/ui_instance_page branch from 47d228f to 0af051b Compare January 17, 2025 02:27
@prha prha force-pushed the prha/ui_pool_reference branch from fefe7fa to fda08ba Compare January 17, 2025 02:54
@prha prha force-pushed the prha/ui_instance_page branch from 0af051b to d199ebd Compare January 17, 2025 02:55
@prha prha force-pushed the prha/ui_pool_reference branch 2 times, most recently from 39f76f5 to dd164aa Compare January 20, 2025 00:28
@prha prha force-pushed the prha/ui_instance_page branch from d199ebd to 8c0784c Compare January 20, 2025 00:28
@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dagster-docs-legacy ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 20, 2025 0:52am

@prha prha changed the base branch from prha/ui_pool_reference to prha/run_pools_ui January 20, 2025 00:28
@prha prha force-pushed the prha/run_pools_ui branch from 3e86416 to 3f350c4 Compare January 21, 2025 17:58
@prha prha force-pushed the prha/ui_instance_page branch from 8c0784c to f9a9f32 Compare January 21, 2025 17:58
@prha prha force-pushed the prha/run_pools_ui branch 2 times, most recently from 44a9628 to 30ce73f Compare January 22, 2025 01:58
@prha prha force-pushed the prha/ui_instance_page branch from f9a9f32 to c72a8e6 Compare January 22, 2025 01:58
@prha prha force-pushed the prha/run_pools_ui branch from 30ce73f to 49a6f83 Compare January 22, 2025 19:04
@prha prha force-pushed the prha/ui_instance_page branch from c72a8e6 to 7b80a1d Compare January 22, 2025 19:05
@prha prha force-pushed the prha/run_pools_ui branch from 49a6f83 to 9f644f3 Compare January 22, 2025 19:26
@prha prha force-pushed the prha/ui_instance_page branch from 7b80a1d to 1ed7d4d Compare January 22, 2025 19:26
@prha prha requested review from bengotow and hellendag and removed request for hellendag January 22, 2025 19:31
Copy link
Copy Markdown
Member

@hellendag hellendag left a comment

Choose a reason for hiding this comment

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

Can you include a few screenshots?

key={item.key}
item={item}
active={item.type === 'link' && pathname === item.path}
active={item.type === 'link' && pathname.startsWith(item.path)}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wanted the Concurrency tab to selected when viewing the detailed page. The link item maps to the index page (without the specific key path).

<InstanceHealthPageContent />
</Route>
<Route path="/deployment/concurrency">
<Route path="/deployment/concurrency(/?.*)">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this be /deployment/concurrency/:concurrencyKey?? Or is the concurrencyKey value too complex for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wanted the URL to match both the top-level /deployment/concurrency url as well as the detailed pool-specific /deployment/concurrency/foo url.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking that using :concurrencyKey? (with the ?) would allow this, but off the top of my head I don't remember whether that's the way to do that with react-router.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh, you're right, that does work

>
<Subheading>In progress</Subheading>
</Box>
<Box style={{marginLeft: -1}}>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does it look like without these?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a left-border for all the table rows, so it looks like an extra thick border. I couldn't figure out how to disable that, so I just cheated 😭

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Screenshot 2025-01-23 at 12 11 01 PM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I guess we have that elsewhere too. Oh well. Probably worth it for me to go in and fix these.

Could have sworn we had resolved that in some way at some point, since all of our tables tend to be full-width...

Comment on lines +203 to +239
if (String(value) !== concurrencyLimit.trim()) {
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the specific case here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think if you tried to do some sort of decimal:

const valueStr = "1.3";
const value = parseInt(valueStr);  // evaluates to 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if you use type="number" on the input, it won't allow a decimal to be entered since the default stepping value is 1 and input is therefore restricted to integers.

@prha prha force-pushed the prha/run_pools_ui branch from 9f644f3 to 9c9353d Compare January 22, 2025 21:11
@prha prha force-pushed the prha/ui_instance_page branch from 1ed7d4d to 47a0fa5 Compare January 22, 2025 21:11
<InstanceHealthPageContent />
</Route>
<Route path="/deployment/concurrency">
<Route path="/deployment/concurrency(/?.*)">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking that using :concurrencyKey? (with the ?) would allow this, but off the top of my head I don't remember whether that's the way to do that with react-router.

>
<Subheading>In progress</Subheading>
</Box>
<Box style={{marginLeft: -1}}>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I guess we have that elsewhere too. Oh well. Probably worth it for me to go in and fix these.

Could have sworn we had resolved that in some way at some point, since all of our tables tend to be full-width...

Comment on lines +95 to +111
<Heading>
<Box flex={{direction: 'row', gap: 8, alignItems: 'center'}}>
<div>
<Link to="/deployment/concurrency">
{flagPoolUI ? 'Pools' : 'Concurrency keys'}
</Link>
</div>
<div>/</div>
<div>{concurrencyKey}</div>
</Box>
</Heading>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the screenshot, this heading section looks like it needs a bottom border.

Comment on lines +203 to +239
if (String(value) !== concurrencyLimit.trim()) {
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if you use type="number" on the input, it won't allow a decimal to be entered since the default stepping value is 1 and input is therefore restricted to integers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants