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

Component library adding global index and other housekeeping #16441

Merged
merged 5 commits into from
Nov 16, 2022

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented Nov 9, 2022

Explanation

What is the current state of things and why does it need to change?
Currently to import a component from the component-library/ you have to import it from it's folder. This means for every component there is a new line of code.

What is the solution your changes offer and how does it work?

  • To allow the ability to import multiple components in one import this PR creates a global index.js file and exports all of the components that currently exists from it.
  • Updates all import documentation to use paths based on import from the ui/pages/ directory e.g ui/pages/page-name/page-name.js folder
  • Re-orders styling imports atomically. Molecule components should be able to overwrite atomic component level css
  • Removes an unused proptype from button-base.js

Screenshots/Screencaps

Before

import { AvatarAccount } from '../../ui/components/component-library/avatar-account';
import { ButtonLink } from '../../ui/components/component-library/button-secondary';
import { ButtonPrimary } from '../../ui/components/component-library/button-primary';
import { ButtonSecondary } from '../../ui/components/component-library/button-secondary';
import { Icon, ICON_NAMES } from '../../ui/components/component-library/icon';
import { TextField } from '../../ui/components/component-library/text-field';

After

import {
  AvatarAccount,
  ButtonLink,
  ButtonPrimary,
  ButtonSecondary,
  Icon,
  ICON_NAMES,
  TextField,
} from '../../ui/components/component-library';

Manual Testing Steps

  • Pull this branch
  • in an app component e.g. ui/components/app/account-list-item/account-list-item.js
  • import a component import { AvatarAccount } from '../../component-library'
  • Run storybook yarn storybook
  • See if the component renders by searching AccountListItem in the search bar

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@georgewrmarshall georgewrmarshall self-assigned this Nov 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@georgewrmarshall georgewrmarshall added area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension IA/NAV labels Nov 9, 2022
@@ -4,7 +4,7 @@
* Reads all the icon svg files in app/images/icons
* and returns an object of icon name key value pairs
* stored in the environment variable ICON_NAMES
* Used with the Icon component in ./ui/component-library/icon
* Used with the Icon component in ./ui/components/component-library/icon/icon.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrects path

/**
* Addition style properties to apply to the button.
*/
style: PropTypes.object,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed from ButtonBase housekeeping

@georgewrmarshall georgewrmarshall marked this pull request as ready for review November 10, 2022 00:28
@georgewrmarshall georgewrmarshall requested review from a team and kumavis as code owners November 10, 2022 00:28
@metamaskbot
Copy link
Collaborator

Builds ready [f4c9c20]
Page Load Metrics (2639 ± 236 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint953577339769369
domContentLoaded182736462619486234
load182736462639492236
domInteractive182736462619486234

highlights:

storybook

@georgewrmarshall georgewrmarshall force-pushed the fix/16440/house-keeping-general branch from f4c9c20 to 9c6daaf Compare November 15, 2022 01:16
@georgewrmarshall georgewrmarshall force-pushed the fix/16440/house-keeping-general branch from 9c6daaf to 1f4c26b Compare November 15, 2022 01:29
@metamaskbot
Copy link
Collaborator

Builds ready [1f4c26b]
Page Load Metrics (2378 ± 161 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9888116216679
domContentLoaded174932282362336161
load174932282378335161
domInteractive174932282362336161
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

highlights:

storybook

Comment on lines +7 to +12
export { Button } from './button';
export { ButtonBase } from './button-base';
export { ButtonIcon } from './button-icon';
export { ButtonLink } from './button-link';
export { ButtonPrimary } from './button-primary';
export { ButtonSecondary } from './button-secondary';
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to what you did with TextFieldBase, it would be nice to export the constants as well.

Suggested change
export { Button } from './button';
export { ButtonBase } from './button-base';
export { ButtonIcon } from './button-icon';
export { ButtonLink } from './button-link';
export { ButtonPrimary } from './button-primary';
export { ButtonSecondary } from './button-secondary';
export { Button, BUTTON_TYPES } from './button';
export { ButtonBase, BUTTON_BASE_SIZES } from './button-base';
export { ButtonIcon, BUTTON_ICON_SIZES } from './button-icon';
export { ButtonLink, BUTTON_LINK_SIZES } from './button-link';
export { ButtonPrimary, BUTTON_PRIMARY_SIZES } from './button-primary';
export { ButtonSecondary, BUTTON_SECONDARY_SIZES } from './button-secondary';

Looks like will need to actually add export of BUTTON_LINK_SIZES, BUTTON_PRIMARY_SIZES, and BUTTON_SECONDARY_SIZES to the index file of their folder.

Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Nov 15, 2022

Choose a reason for hiding this comment

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

Yeah, I can add any that are already exported from the index. Then we could add all the missing consts as apart of another housekeeping round? Otherwise this PR will just need to be constantly updated and waiting on others?

@georgewrmarshall georgewrmarshall merged commit 23b412c into develop Nov 16, 2022
@georgewrmarshall georgewrmarshall deleted the fix/16440/house-keeping-general branch November 16, 2022 22:15
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🧹 [UI House Keeping] General
4 participants