view_components icon indicating copy to clipboard operation
view_components copied to clipboard

Published 20 hours ago • verified publisher icon primer

Standardize component naming

Open manuelpuyol opened this issue 4 years ago • 4 comments

Overview

We don't have a standard for our component names, having three different variants:

  • Primer::ButtonComponent - with suffix
  • Primer::AutoComplete - no suffix
  • Primer::Alpha::ButtonMarketing - status as a module

This causes confusion and a not ideal developer experience, so we should have a single naming standard.

Proposal

Rename all components to have their status as a module and remove the Component suffix.

Examples:

  • Primer::ButtonComponent -> Primer::Beta::Button
  • Primer::AutoComplete -> Primer::Beta::AutoComplete

Removing the suffix will make working with components less verbose. Using the status as a module will help with status promotion because we can keep the previous version for some time, giving users time to migrate.

How to migrate a component

The majority of the work is done through the component_status_migrator thor command. This command will move files, update references, create a deprecated component in place of the original, fix navigation in storybook and the doc site, and more.

Run a migration

As an example, the above referenced Primer::ButtonComponent can be migrated with the following command line:

bundle exec thor component_status_migrator ButtonComponent --status beta

Note that this command specifically calls out the target status for the component in --status beta. If this option is left off, it will default to alpha and move the component into the corresponding alpha folder and Alpha:: namespace.

Be sure to set the correct status when migrating a component. Valid status entries include:

  • alpha
  • beta
  • stable
  • deprecated (not fully supported yet)

Testing a migration

Prior to creating a pull request and marking it as ready for review, be sure to run all tests, check storybook, and check the docs site to ensure all updates have been made:

  • run script/test
  • run script/dev
    • go to http://localhost:5000 to check storybook
    • go to localhost:5400 to check the doc site

Common errors

You may run into errors with storybook or the docs site, due to previously built site information. For example, storybook may still have the original Primer::ButtonComponent entry while also having the updated Primer::Beta::Button entry. If this happens, you will need to clean out the build cache and rebuild. One way to do that is to run the following:

  1. git clean -df
  2. git clean -dfx
  3. script/setup

The first call to clean will remove all untracked files and folders that are not part of .gitignore. The second call will remove all untracked files and folders that are part of .gitignore. This will remove everything, including nod_modules and bundle vendor folders. After that, you'll need to run script/setup to reinstall and rebuild before running script/dev again.

Example pull requests

See the component migration work that is already completed, below, for example pull requests.

Work TODO

component migrator code changes:

  • [x] update component_status_migrator.thor script to handle --status stable https://github.com/primer/view_components/pull/1262
  • [x] automate additional manual tasks for replacing references and updating files https://github.com/primer/view_components/pull/1269
  • [x] correct the docs site nav.yml when migrating a component https://github.com/primer/view_components/pull/1276
  • [ ] update component_status_migrator.thor to support migrating from alpha or beta folder, to correct status destination https://github.com/primer/view_components/issues/1263

component migrations:

  • [x] Primer::AutoComplete https://github.com/primer/view_components/pull/678
  • [x] Primer::AutoComplete::Input https://github.com/primer/view_components/pull/678
  • [x] Primer::AutoComplete::Item https://github.com/primer/view_components/pull/678
  • [x] Primer::AvatarComponent https://github.com/primer/view_components/pull/687
  • [x] Primer::AvatarStackComponent https://github.com/primer/view_components/pull/707
  • [x] Primer::BaseButton https://github.com/primer/view_components/pull/1243
  • [ ] Primer::BaseComponent
  • [x] Primer::BlankslateComponent (deprecated version is still live, due to API changes)
  • [x] Primer::BorderBoxComponent https://github.com/primer/view_components/pull/1255
  • [x] Primer::BoxComponent https://github.com/primer/view_components/pull/1266
  • [x] Primer::BreadcrumbComponent
  • [x] Primer::BreadcrumbComponent::ItemComponent
  • [x] Primer::ButtonComponent https://github.com/primer/view_components/pull/1271
  • [x] Primer::ButtonGroup https://github.com/primer/view_components/pull/1275
  • [ ] Primer::ClipboardCopy
  • [x] Primer::CloseButton https://github.com/primer/view_components/pull/1277
  • [x] Primer::CounterComponent https://github.com/primer/view_components/pull/1279
  • [x] Primer::DetailsComponent https://github.com/primer/view_components/pull/1280
  • [ ] Primer::Dropdown
  • [ ] Primer::Dropdown::Menu
  • [ ] Primer::Dropdown::Menu::Item
  • [ ] Primer::DropdownMenuComponent
  • [x] Primer::FlashComponent
  • [ ] Primer::FlexComponent
  • [ ] Primer::FlexItemComponent
  • [x] Primer::HeadingComponent https://github.com/primer/view_components/pull/1297
  • [x] Primer::HiddenTextExpander https://github.com/primer/view_components/pull/1303
  • [ ] Primer::IconButton https://github.com/primer/view_components/pull/1397
  • [ ] Primer::Image
  • [ ] Primer::ImageCrop
  • [ ] Primer::LabelComponent
  • [ ] Primer::LayoutComponent
  • [ ] Primer::LinkComponent
  • [ ] Primer::LocalTime
  • [ ] Primer::Markdown
  • [ ] Primer::MenuComponent
  • [ ] Primer::Navigation::TabComponent
  • [ ] Primer::OcticonComponent
  • [ ] Primer::OcticonSymbolsComponent
  • [ ] Primer::PopoverComponent
  • [ ] Primer::ProgressBarComponent
  • [ ] Primer::SpinnerComponent
  • [ ] Primer::StateComponent
  • [ ] Primer::SubheadComponent
  • [ ] Primer::TabContainerComponent
  • [ ] Primer::TabNavComponent
  • [ ] Primer::TimeAgoComponent
  • [ ] Primer::TimelineItemComponent
  • [ ] Primer::TimelineItemComponent::BadgeComponent
  • [ ] Primer::Tooltip
  • [ ] Primer::Truncate
  • [ ] Primer::UnderlineNavComponent

manuelpuyol avatar Jul 02 '21 15:07 manuelpuyol

I made an embarrassingly nasty script in https://github.com/primer/view_components/pull/701 that we can hack on that I think should save us a bunch of time in this work.

srt32 avatar Jul 21 '21 22:07 srt32

This issue has been getting stale for a while, and having a mix of naming conventions in the codebase is confusing for us library engineers and the developers using it.

I think it would be good to make a concerted effort to migrate the component names (possibly in one dedicated release?) and be done with it. Manuel and I have been working on changes to the migration script so that we can alias the old name to the new name, thereby running the migration in a non-breaking way. See https://github.com/primer/view_components/pull/975

I hope we have the script complete and tested this week so we can start the migration next week if we're all happy.

pouretrebelle avatar Jan 17 '22 15:01 pouretrebelle

Yesterday @hectahertz and I finished #975 so we're technically prepared for running these migrations, but we started thinking about the consequences of doing all these renames and didn't feel confident that the status of each component actually corresponds to the status criteria.

A lot of the components were upgraded to beta based on old standards that didn't have acceptance criteria for accessibility or API reviews, and some may already meet the criteria for the next level. We decided that it's important for us to feel confident about all the statuses before integrating them into the component names.

Our plan is to implement the component status checklist as currently used in Primer React (see https://github.com/github/primer/issues/339#issuecomment-983925993) and audit each component to assign an accurate status to it. We don't plan on remediating any issues apart from minor issues that are preventing the component from progressing to the next level. This gives @hectahertz and I a chance to familiarise ourselves with all our components and will make it much easier to report on maturity progression going forward. I'll reference the tracking issues here when I write them.

cc: @yaili @colebemis

pouretrebelle avatar Jan 18 '22 11:01 pouretrebelle

Since https://github.com/primer/view_components/pull/1441 was merged, Primer::FlexComponent and Primer::FlexItemComponent should be removed.

ollie-iterators avatar Oct 04 '22 17:10 ollie-iterators

The components Primer::LocalTime and Primer::TimeAgoComponent may be able to be removed from this issue once #1650 is merged.

ollie-iterators avatar Dec 02 '22 15:12 ollie-iterators

Since https://github.com/primer/view_components/pull/1650 was merged, Primer::LocalTime and Primer::TimeAgoComponent can be removed from this issue.

ollie-iterators avatar Dec 06 '22 18:12 ollie-iterators

Why is Primer::LinkComponent migrated to Primer::Beta::LinkComponent? I thought that the Component suffix should be removed from all of the components.

ollie-iterators avatar Dec 08 '22 19:12 ollie-iterators

@ollie-iterators

Why is Primer::LinkComponent migrated to Primer::Beta::LinkComponent? I thought that the Component suffix should be removed from all of the components.

oops! that was a mistake in the title of the PR. the component is Primer::Beta::Link. i've updated the PR title. don't know how long it will take for the link here to reflect that

mxriverlynn avatar Dec 08 '22 19:12 mxriverlynn

The Pull Request: https://github.com/primer/view_components/pull/1729 was merged, so Primer::TimelineItemComponent::BadgeComponent can be marked as completed.

ollie-iterators avatar Dec 30 '22 17:12 ollie-iterators

Since https://github.com/primer/view_components/pull/1730 was merged, Primer::Navigation::TabComponent's check box should be checked off.

ollie-iterators avatar Mar 02 '23 16:03 ollie-iterators

Since https://github.com/primer/view_components/pull/1877 was merged, Primer::Truncate can be checked off.

ollie-iterators avatar Mar 13 '23 18:03 ollie-iterators

Thanks @ollie-iterators. @hrs can you confirm https://github.com/primer/view_components/issues/676#issuecomment-1466713881 has been completed and update the summary task list if so?

lesliecdubs avatar Mar 17 '23 22:03 lesliecdubs

I don't think Primer::LayoutComponent has been migrated yet. It doesn't look like it has a status of deprecated in https://github.com/primer/view_components/blob/main/app/components/primer/layout_component.rb

ollie-iterators avatar Mar 19 '23 16:03 ollie-iterators

Yup, that's all right! Primer::Truncate has been migrated and checked off. Primer::LayoutComponent hasn't yet been deprecated but will be shortly!

hrs avatar Mar 20 '23 20:03 hrs

Also, I don't know if https://github.com/primer/view_components/issues/1263 has been fixed. There was a PR that was related to it that was merged, but it is still open.

ollie-iterators avatar Mar 21 '23 16:03 ollie-iterators

Nope, but I'm working on that one right this minute. Ought to be done before too much longer. 😄

hrs avatar Mar 21 '23 16:03 hrs

Since https://github.com/primer/view_components/issues/1263 was closed, this Issue can be closed as completed.

ollie-iterators avatar Mar 30 '23 20:03 ollie-iterators