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

refactor: resource preset page #2656

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Conversation

agatha197
Copy link
Contributor

@agatha197 agatha197 commented Aug 22, 2024

Note

Unlike other components, the requiredMark="optional" is not included to form. Because it contains too many optional inputs.
image

resolves #2651

TL;DR

Implemented a new ResourcePresetList, ResourcePresetSettingModal component and updated the EnvironmentPage to use it.

What changed?

  • Added a new ResourcePresetList component to manage resource presets
  • Created a ResourcePresetSettingModal component for creating and editing resource presets
  • Updated the EnvironmentPage to use the new ResourcePresetList component
  • Adjusted padding and styling in ContainerRegistryList and KeypairResourcePolicyList
  • Added new translations for resource preset-related strings

How to test?

  1. Navigate to the Environment page
  2. Select the "Resource Presets" tab
  3. Verify that the new ResourcePresetList is displayed
  4. Test creating, editing, and deleting resource presets
  5. Confirm that the list updates correctly after each action
  6. Check that the shared memory validation works as expected
  7. Check that the new session is created successfully with the newly added resource preset.

Why make this change?

This change improves the user interface for managing resource presets, providing a more consistent and user-friendly experience. It also integrates better with the existing components and allows for easier maintenance and future enhancements of the resource preset functionality.

Screenshots

  • List
    image.png

  • Modify

image.png

  • Create

image.png

  • Dark mode
    image.png

  • Validations
    image.png

  • Confirm to delete

image.png


Checklist: (if applicable)

  • Mention to the original issue
  • Documentation
  • Minium required manager version: 23.09
  • Specific setting for review (eg., KB link, endpoint or how to setup)
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

Copy link

graphite-app bot commented Aug 22, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the size:L 100~500 LoC label Aug 22, 2024
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @agatha197 and the rest of your teammates on Graphite Graphite

@agatha197 agatha197 force-pushed the refactor/resource-preset-page branch from ef84be4 to 0c4d3e9 Compare August 23, 2024 05:16
@github-actions github-actions bot added area:ux UI / UX issue. area:i18n Localization size:XL 500~ LoC and removed size:L 100~500 LoC labels Aug 23, 2024
@agatha197 agatha197 force-pushed the refactor/resource-preset-page branch from 0c4d3e9 to ab86c7a Compare August 23, 2024 05:52
Copy link

github-actions bot commented Aug 23, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
5.34% (-0.08% 🔻)
340/6367
🔴 Branches
4.83% (-0.08% 🔻)
214/4433
🔴 Functions
3% (-0.06% 🔻)
63/2099
🔴 Lines
5.24% (-0.08% 🔻)
326/6220
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / ResourcePresetList.tsx
0% 0% 0% 0%
🔴
... / ResourcePresetSettingModal.tsx
0% 0% 0% 0%

Test suite run success

90 tests passing in 11 suites.

Report generated by 🧪jest coverage report action from 0c00d60

@agatha197 agatha197 marked this pull request as ready for review August 23, 2024 06:00
@agatha197 agatha197 force-pushed the refactor/resource-preset-page branch from ab86c7a to 2cb2e98 Compare August 23, 2024 06:48
@agatha197 agatha197 added effort:normal Need to understand a few modules / some extent of contextual or historical information. field:UI / UX impact:visible This change is visible to users. platform:general type:refactor Refactoring current implementation. urgency:2 With time limit, it should be finished within it; otherwise, resolve it when no other chores. labels Aug 23, 2024 — with Graphite App
@agatha197 agatha197 force-pushed the refactor/resource-preset-page branch from 2cb2e98 to c126615 Compare August 28, 2024 04:26
Copy link
Member

@gahyuun gahyuun left a comment

Choose a reason for hiding this comment

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

I get this error when I create a preset with a duplicate name.
Don't need to handle error?
image

react/src/pages/EnvironmentPage.tsx Show resolved Hide resolved
react/src/components/ResourcePresetList.tsx Show resolved Hide resolved
Copy link
Member

@gahyuun gahyuun left a comment

Choose a reason for hiding this comment

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

.

@agatha197 agatha197 force-pushed the refactor/resource-preset-page branch 2 times, most recently from c64148c to f730b33 Compare August 30, 2024 05:39
@agatha197 agatha197 requested a review from gahyuun August 30, 2024 05:39
Copy link
Contributor

@ironAiken2 ironAiken2 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

graphite-app bot commented Sep 30, 2024

Merge activity

> [!Note]
> Unlike other components, the `requiredMark="optional"` is not included to form. Because it contains too many optional inputs.
> <img width="300" alt="image" src="https://github.com/user-attachments/assets/12f4c0cf-3bbc-4b4b-bacb-94e2612d83b5">

resolves #2651

### TL;DR

Implemented a new ResourcePresetList, ResourcePresetSettingModal component and updated the EnvironmentPage to use it.

### What changed?

- Added a new ResourcePresetList component to manage resource presets
- Created a ResourcePresetSettingModal component for creating and editing resource presets
- Updated the EnvironmentPage to use the new ResourcePresetList component
- Adjusted padding and styling in ContainerRegistryList and KeypairResourcePolicyList
- Added new translations for resource preset-related strings

### How to test?

1. Navigate to the Environment page
2. Select the "Resource Presets" tab
3. Verify that the new ResourcePresetList is displayed
4. Test creating, editing, and deleting resource presets
5. Confirm that the list updates correctly after each action
6. Check that the shared memory validation works as expected
7. Check that the new session is created successfully with the newly added resource preset.

### Why make this change?

This change improves the user interface for managing resource presets, providing a more consistent and user-friendly experience. It also integrates better with the existing components and allows for easier maintenance and future enhancements of the resource preset functionality.

### Screenshots
- List
![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/2HueYSdFvL8pOB5mgrUQ/524e6eca-1bc8-4c35-bd5c-7495974c616c.png)

- Modify

![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/2HueYSdFvL8pOB5mgrUQ/5f9f7184-4b77-4c95-b693-96f954d8d29e.png)

- Create

![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/2HueYSdFvL8pOB5mgrUQ/532a11d9-1523-4d49-b5db-4147bfc30cb2.png)

- Dark mode
![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/2HueYSdFvL8pOB5mgrUQ/1bded613-e2a4-4db2-b240-dcda526b4411.png)

- Validations
![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/2HueYSdFvL8pOB5mgrUQ/b23041e4-a73c-4731-af6e-3ee8223213de.png)

- Confirm to delete

![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/2HueYSdFvL8pOB5mgrUQ/b4e88afd-60a2-4e9e-854e-2c9c3ced0a1d.png)

---

<!--
Please precisely, concisely, and concretely describe what this PR changes, the rationale behind codes,
and how it affects the users and other developers.
-->

**Checklist:** (if applicable)

- [ ] Mention to the original issue
- [ ] Documentation
- [x] Minium required manager version: 23.09
- [x] Specific setting for review (eg., KB link, endpoint or how to setup)
- [x] Minimum requirements to check during review
- [ ] Test case(s) to demonstrate the difference of before/after
@graphite-app graphite-app bot merged commit 0c00d60 into main Sep 30, 2024
7 checks passed
@graphite-app graphite-app bot deleted the refactor/resource-preset-page branch September 30, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:i18n Localization area:ux UI / UX issue. effort:normal Need to understand a few modules / some extent of contextual or historical information. field:UI / UX impact:visible This change is visible to users. platform:general size:XL 500~ LoC type:refactor Refactoring current implementation. urgency:2 With time limit, it should be finished within it; otherwise, resolve it when no other chores.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generalize accelerator input parts of resource preset
4 participants