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.

Sign up for GitHub

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

Jump to bottom

refactor: Finish organizing components #9488

Merged
merged 7 commits into from
Aug 4, 2023
Merged

refactor: Finish organizing components #9488

merged 7 commits into from
Aug 4, 2023

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Aug 3, 2023 •
edited
Loading

Description 📝

  • This PR's goal is to finish deprecating the src/components/core folder to bring consistency to how we consume components 🎉 📁
    • This means we can finally just trust src/components/[name] as the single source of truth 🥳
  • I know these PRs might be getting annoying but this should be the last one!!

Preview 📷

Before 😵‍💫

import React, { useEffect, useState } from 'react';

import { Notice } from 'src/components/Notice/Notice';
import { Typography } from 'src/components/Typography';
import { Paper } from 'src/components/Paper';
import { Tab } from 'src/components/core/ReachTab';
import { TabList } from 'src/components/core/ReachTabList';
import TabPanel from 'src/components/core/ReachTabPanel';
import TabPanels from 'src/components/core/ReachTabPanels';
import Tabs from 'src/components/core/ReachTabs';

export interface Tab {
...

After 🧼

import React, { useEffect, useState } from 'react';

import { Notice } from 'src/components/Notice/Notice';
import { Paper } from 'src/components/Paper';
import { Tab } from 'src/components/ReachTab';
import { TabList } from 'src/components/ReachTabList';
import { TabPanel } from 'src/components/ReachTabPanel';
import { TabPanels } from 'src/components/ReachTabPanels';
import { Tabs } from 'src/components/ReachTabs';
import { Typography } from 'src/components/Typography';

export interface Tab {
...

How to test 🧪

Important

There shouldn't be much manual testing needed. This PR mostly just updates imports and file names.

  • Verify automated checks pass
  • Glance over the diff
  • Verify Cloud Manager works as expected

@bnussman-akamai bnussman-akamai self-assigned this Aug 3, 2023
@bnussman-akamai bnussman-akamai changed the title refactor: Finish deprecating the core folder refactor: Finish organizing components Aug 3, 2023
mjac0bs
mjac0bs approved these changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for the clean up and organization! 🎉 🧹 Didn't notice anything amiss in this diff or Cloud.

packages/manager/src/components/CircularProgress.tsx

/**
* Not to be confused with `<CircleProgress />`.
* @todo Consolidate these two components
Copy link
Contributor

Choose a reason for hiding this comment

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

++ to consolidation. Can we get a ticket into the backlog for this, if we don't have one already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Created M3-6962

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Aug 4, 2023
coliu-akamai
coliu-akamai approved these changes Aug 4, 2023
View reviewed changes
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.

looks good! 🎉

packages/manager/src/components/Breadcrumb/FinalCrumbPrefix.tsx
type CombinedProps = Props;

const FinalCrumbPrefix: React.FC<CombinedProps> = (props) => {
export const FinalCrumbPrefix = (props: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need the React.memo here or is this component small enough that it doesn't really affect it too much? Same with CookieWarning.tsx

Copy link
Member Author

Choose a reason for hiding this comment

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

I think because this component is rarely rendered, no memoization is fine here. It probably was an over optimization.

packages/manager/src/components/Breadcrumb/FinalCrumbPrefix.tsx
@@ -1,6 +1,5 @@
import { makeStyles } from '@mui/styles';
import * as React from 'react';
import { compose } from 'recompose';
Copy link
Contributor

Choose a reason for hiding this comment

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

(not part of this ticket at all so feel free to ignore) worth getting rid of themakeStyles from '@mui/styles' from this file and use the tss codemod or styled api? (or putting up a backlog ticket for refactoring this component's styles)

Copy link
Member Author

Choose a reason for hiding this comment

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

Created M3-6963

packages/manager/src/components/EditableEntityLabel/EditableInput.tsx
import { TextField, TextFieldProps } from 'src/components/TextField';
import { Typography } from 'src/components/Typography';
import ClickAwayListener from 'src/components/core/ClickAwayListener';
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as FinalCrumbPrefix.tsx -- feel free to ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Created M3-6964

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 4, 2023
@carrillo-erik
Copy link
Contributor

Looks good on my end. Thanks for taking on this task and improving the repo's quality.

@bnussman-akamai bnussman-akamai merged commit 27497b0 into linode:develop Aug 4, 2023
jaalah-akamai pushed a commit that referenced this pull request Aug 7, 2023
@bnussman-akamai @bnussman @jaalah
refactor: Finish organizing components (#9488)
efc6228 
* finish deprecating the `core` folder

* storybook story improvments

* Added changeset: Finish deprecating the `core` folder and clean up components

* clean up `LandingHeader`

* clean up some default exports and `recompose` usages

* fix unit tests

* remove extra default exports

---------

Co-authored-by: Banks Nussman <[email protected]>
jaalah-akamai pushed a commit that referenced this pull request Aug 8, 2023
@bnussman-akamai @bnussman @jaalah
refactor: Finish organizing components (#9488)
e556e59 
* finish deprecating the `core` folder

* storybook story improvments

* Added changeset: Finish deprecating the `core` folder and clean up components

* clean up `LandingHeader`

* clean up some default exports and `recompose` usages

* fix unit tests

* remove extra default exports

---------

Co-authored-by: Banks Nussman <[email protected]>
This was referenced Aug 17, 2023
Merged
Merged
@coliu-akamai coliu-akamai mentioned this pull request Aug 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjac0bs mjac0bs mjac0bs approved these changes

@carrillo-erik carrillo-erik carrillo-erik approved these changes

@jaalah-akamai jaalah-akamai jaalah-akamai approved these changes

@coliu-akamai coliu-akamai coliu-akamai approved these changes

@tyler-akamai tyler-akamai Awaiting requested review from tyler-akamai

@jdamore-linode jdamore-linode Awaiting requested review from jdamore-linode

@dwiley-akamai dwiley-akamai Awaiting requested review from dwiley-akamai

@hana-akamai hana-akamai Awaiting requested review from hana-akamai

Assignees

@bnussman-akamai bnussman-akamai

Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bnussman-akamai @carrillo-erik @mjac0bs @jaalah-akamai @coliu-akamai @bnussman