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: [M3-6063] - Refactor components to use TypeToConfirmDialog component #9175

Merged
merged 30 commits into from
Jul 5, 2023

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented May 25, 2023

Description 📝

Refactor components to standardize type-to-confirm implementation using the TypeToConfirmDialog component.

Major Changes 🔄

  • Introduced the following new types in the EntityInfo interface: Database, Kubernetes, AccountSetting.
  • Introduced the subType property in the EntityInfo interface: Backup, Cluster, ObjectStorage, CloseAccount.

How to test 🧪

  1. Verify type-to-confirm functionality across the application.
  2. Check for any visual regressesions.

@cypress
Copy link

cypress bot commented May 25, 2023

3 flaky tests on run #4714 ↗︎

0 177 3 0 Flakiness 3

Details:

Merge branch 'develop' of https://github.com/linode/manager into refactor/M3-606...
Project: Cloud Manager E2E Commit: c2ce6a678e
Status: Passed Duration: 15:15 💡
Started: Jul 3, 2023 8:18 PM Ended: Jul 3, 2023 8:33 PM
Flakiness  linodes/linode-storage.spec.ts • 1 flaky test

View Output Video

Test Artifacts
linode storage tab > delete disk Output Screenshots Video
Flakiness  stackscripts/create-stackscripts.spec.ts • 1 flaky test

View Output Video

Test Artifacts
stackscripts > creates a StackScript with Any/All target image Output Screenshots Video
Flakiness  linodes/smoke-linode-landing-table.spec.ts • 1 flaky test

View Output Video

Test Artifacts
linode landing actions > deleting multiple linodes with action menu Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@linode linode deleted a comment from bnussman May 25, 2023
@linode linode deleted a comment from bnussman May 25, 2023
@abailly-akamai
Copy link
Contributor

@carrillo-erik not sure what you are trying to do with passing {...rest} to TypeToConfirmDialog? Maybe start by deleting that and work your way from there?

Screenshot 2023-05-30 at 1 47 03 PM

@carrillo-erik carrillo-erik changed the title WIP refactor: [M3-6063] - Refactor components to use TypeToConfirmDialog component Jun 2, 2023
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Nice! needs some cleanup (will review in the browser when done)

Also, don't hesitate to add a comment why you are using a flex order because of the renderProps

@carrillo-erik carrillo-erik self-assigned this Jun 7, 2023
@carrillo-erik carrillo-erik added Add'tl Approval Needed Waiting on another approval! and removed Requires Changes labels Jun 29, 2023
@bnussman-akamai bnussman-akamai added Requires Changes and removed Add'tl Approval Needed Waiting on another approval! labels Jun 29, 2023
Copy link
Member

@bnussman-akamai bnussman-akamai 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 checkout the PR and run yarn, then push the changes. The yarn.lock is out of date

@jaalah-akamai
Copy link
Contributor

jaalah-akamai commented Jun 30, 2023

@carrillo-erik There has come together really nice. I think this component will benefit a ton from a storybook page. See https://github.com/linode/manager/tree/develop/packages/manager/src/components/Button as to how we did the JSDocs and stories file.

Implementation looks good apart from this

@carrillo-erik
Copy link
Contributor Author

@bnussman-akamai I had pushed some changes to the yarn.lock file and the cachedData files. John asked me to remove those changes from the PR. I reverted the commit and reintroduced the changes to fix the pertaining issue.

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Functionality of all improved components looks good. Approved pending suggestions from @jaalah-akamai @bnussman-akamai.

Nice work and thanks for bearing with the review changes!

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jun 30, 2023
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jun 30, 2023
@bnussman-akamai
Copy link
Member

Sounds good @carrillo-erik, still before you merge, just run yarn and push those changes. If we don't we'll get dependabot alerts

@carrillo-erik
Copy link
Contributor Author

carrillo-erik commented Jul 3, 2023

Can you checkout the PR and run yarn, then push the changes. The yarn.lock is out of date

@bnussman-akamai

I had some merge conflicts with the yarn.lock file after pulling the latest. I believe this resolves the issue you brought up. Let me know, thanks.

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants