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-8648] – Migrate several components to ui package prior to migrating Paper #11159

Merged

Conversation

dwiley-akamai
Copy link
Contributor

Description 📝

Migrate the following components from manager to ui:

  • FormControl
  • FormHelperText
  • Input
  • InputAdornment
  • InputLabel

This PR is the result of a cascade of dependencies when I initially tried migrating Paper, which imports FormHelperText. I wanted to avoid a giant 200+ file PR so this PR sets the groundwork for a follow-up PR focused on migrating Paper by handling the cascade of dependencies and cleaning up TS errors.

Target release date 🗓️

11/12/24

How to test 🧪

Verification steps

  • Ensure there are no regressions or crashes in Cloud
  • All checks should pass (flakes aside)

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@dwiley-akamai dwiley-akamai self-assigned this Oct 24, 2024
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner October 24, 2024 21:41
@dwiley-akamai dwiley-akamai requested review from hkhalil-akamai and jaalah-akamai and removed request for a team October 24, 2024 21:41
@hkhalil-akamai
Copy link
Contributor

I think I set the wrong precedent with Tooltip, but what do we think about making a directory for components with multiple supporting files?

For example, we could create a directory FormControl which contains both FormControl.tsx and FormControl.stories.tsx, along with an index.ts file to re-export the component? This would help in keeping the components directory neat.

@dwiley-akamai
Copy link
Contributor Author

I think I set the wrong precedent with Tooltip, but what do we think about making a directory for components with multiple supporting files?

For example, we could create a directory FormControl which contains both FormControl.tsx and FormControl.stories.tsx, along with an index.ts file to re-export the component? This would help in keeping the components directory neat.

We have a mix of both approaches in manager/src/components right now. We could go either way with it but since it's just the component and its story (I don't think unit test files are getting migrated, right?), I don't mind not having the extra directory layer.

@hkhalil-akamai
Copy link
Contributor

We have a mix of both approaches in manager/src/components right now. We could go either way with it but since it's just the component and its story (I don't think unit test files are getting migrated, right?), I don't mind not having the extra directory layer.

Sounds good, I'd be interested to hear what others think though. I believe we are migrating unit tests though, for components that have them.

@mjac0bs
Copy link
Contributor

mjac0bs commented Oct 25, 2024

Sounds good, I'd be interested to hear what others think though. I believe we are migrating unit tests though, for components that have them.

Just chiming in to say that I like directories for components and their related files a lot. The loose files kind of stress me out. 😅 We actually recommend the dirs as a best practice in our docs: https://linode.github.io/manager/development-guide/01-repository-structure.html#where-to-go-for-common-tasks.

Screenshot 2024-10-25 at 11 43 58 AM

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.

Components working as expected. Approved pending decision on whether to make component folders.

@dwiley-akamai
Copy link
Contributor Author

Just chiming in to say that I like directories for components and their related files a lot. The loose files kind of stress me out. 😅 We actually recommend the dirs as a best practice in our docs: https://linode.github.io/manager/development-guide/01-repository-structure.html#where-to-go-for-common-tasks.

Good catch that it's in the docs, I think we should abide by that then.

Copy link

github-actions bot commented Oct 25, 2024

Coverage Report:
Base Coverage: 87.04%
Current Coverage: 87.05%

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

thanks Dajahi! 🎉

✅ checked throughout CM, didn't see any page crashes/things looked the same

packages/manager/src/components/TextField.tsx Outdated Show resolved Hide resolved
@coliu-akamai coliu-akamai added the Approved Multiple approvals and ready to merge! label Oct 28, 2024
@dwiley-akamai dwiley-akamai merged commit 7a17a78 into linode:develop Oct 29, 2024
23 checks passed
@dwiley-akamai dwiley-akamai deleted the M3-8648-prelude-paper-ui-package branch October 29, 2024 14:09
Copy link

cypress bot commented Oct 29, 2024

Cloud Manager E2E    Run #6749

Run Properties:  status check passed Passed #6749  •  git commit 7a17a78f90: refactor: [M3-8648] – Migrate several components to `ui` package prior to migrat...
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6749
Run duration 25m 43s
Commit git commit 7a17a78f90: refactor: [M3-8648] – Migrate several components to `ui` package prior to migrat...
Committer Dajahi Wiley
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 445
View all changes introduced in this branch ↗︎

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! Modularization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants