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!: Remove 1.x.x and 2.x.x deprecated properties #1988

Merged
merged 21 commits into from
Apr 21, 2022

Conversation

brandonlenz
Copy link
Contributor

@brandonlenz brandonlenz commented Apr 15, 2022

Summary

Related Issues or PRs

closes: #1946

How To Test

The following components that used to have deprecated props do not issue any deprecation warnings:

  • Accordion
  • Alert
  • Button
  • CollectionHeading
  • Footer/Address
  • Footer/Footer
  • Footer/FooterNav
  • Footer/Logo
  • Search
  • Fieldset
  • TextInput
  • header/NavList
  • StepIndicator

The storybook components that previously relied on default heading levels render:

  • Accordion
  • Alert
  • CollectionHeading
  • StepIndicator

Screenshots (optional)

/**
* @deprecated since 1.6.0, use validationStatus
*/
success?: boolean
inputSize?: 'small' | 'medium'
Copy link
Contributor Author

@brandonlenz brandonlenz Apr 15, 2022

Choose a reason for hiding this comment

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

We actually don't use this prop at all in storybook, and USWDS doesn't show it in their examples, though it comes from the following:

image

Wondering if we should create a type that enumerates those valid sizes, or just leave it up to consumers to pass in the desired className.

If the latter is chosen, I can remove the prop here and call it out as part of the breaking release. I think we should do something because the prop doesn't offer all the options that uswds intends.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been meaning to see what it would look like to create a nice interface for 2xs...2xl passed in as a size prop on a couple of components, which would just append size to the className like so:

`usa-input--${size}`: size

I think its a nice convenience to offer, and probably doesn't involve much overhead on our part besides combing through the code. So that's what the alternative might look like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I like that. I think my only 🤔 was if that would make maintenance harder because we would be using dynamically calculated class names instead of having to list out all the potential options ourselves in a classnames object and having it readable in the code. But thinking about it more, that's probably what storybook is for - we can show all the variants of things there.

@brandonlenz brandonlenz changed the title BREAKING CHANGE: Remove 1.x.x and 2.x.x deprecated features BREAKING CHANGE: Remove 1.x.x and 2.x.x deprecated properties Apr 15, 2022
@brandonlenz brandonlenz changed the title BREAKING CHANGE: Remove 1.x.x and 2.x.x deprecated properties chore!: Remove 1.x.x and 2.x.x deprecated properties Apr 15, 2022
Comment on lines -23 to -24
"**/*.test.ts",
"**/*.test.tsx",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@suzubara I think you'd be the best person to ask about this.

So when these are excluded from tsconfig, typescript can't pickup custom types we define within tests like HeadingLevel from headingLevel.d.ts

Things don't seem to break by including it, but I'm sure there was a reason to exclude tests, so I wanted to check with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

My haphazard guess is maybe we included this to not have type issues in tests just break typescript compile for the rest of the lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that sounds right. I could see it being an annoying if you're just trying to write some tests quickly and focus on the typing where it really matters. Given that it works when I've removed these exclusions, it seems like we're in pretty good shape. I definitely don't want to harm anyone's developer experience though

@brandonlenz
Copy link
Contributor Author

Jest: "global" coverage threshold for branches (87%) not met: 86.99%

😅

Comment on lines -68 to -76
/*
export const small = (): React.ReactElement => (
<TextInput id="input-type-text" name="input-type-text" type="text" inputSize="small" />
)

export const medium = (): React.ReactElement => (
<TextInput id="input-type-text" name="input-type-text" type="text" inputSize="medium" />
)
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deletion of these commented stories corresponds to #1988 (comment)

haworku
haworku previously approved these changes Apr 19, 2022
@brandonlenz brandonlenz changed the title chore!: Remove 1.x.x and 2.x.x deprecated properties refactor!: Remove 1.x.x and 2.x.x deprecated properties Apr 20, 2022
hgarfinkle
hgarfinkle previously approved these changes Apr 21, 2022
hgarfinkle
hgarfinkle previously approved these changes Apr 21, 2022
@brandonlenz brandonlenz merged commit 5dfadb1 into main Apr 21, 2022
@brandonlenz brandonlenz deleted the bl-remove-deprecation-warnings branch April 21, 2022 21:30
brandonlenz added a commit that referenced this pull request Apr 25, 2022
## [3.0.0](2.9.0...3.0.0) (2022-04-25)


### ⚠ BREAKING CHANGES

* Previous deprecated features and props have been removed. Please see the following guidance for affected components:
  * Accordion: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop.
  * Alert: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop.
  * Button: 
    * `accent` has been removed. Use `accentStyle` instead.
    * `big`, `small`, and `size="small"` have been removed. Use `size="big"` or do not define the `size` prop for default sizing.
  * CollectionHeading: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop.
  * Footer/Address: `big`, `medium`, and `slim` props have been removed. Use the `size` prop instead.
  * Footer/Footer: `big`, `medium`, and `slim` props have been removed. Use the `size` prop instead.
  * Footer/FooterNav: `big`, `medium`, and `small` props have been removed. Use the `size` prop instead.
  * Footer/Logo: `big`, `medium`, and `small` props have been removed. Use the `size` prop instead.
  * Search: `big`, and `small` props have been removed. Use the `size` prop instead.
  * Fieldset: `legendSrOnly` has been removed. Use `legendStyle="srOnly"` instead.
  * TextInput: `error`, and `success` props have been removed. Use the `validationStatus` prop instead.
  * header/NavList: `primary`, `secondary`, `subnav`, `megamenu`, and `footerSecondary` props have been removed. Use the `type` prop instead.
  * StepIndicator: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop.
* SummaryBox now exposes sub-components (SummaryBoxHeading and SummaryBoxContent) for a more compositional API. Consumers will need to update their implementation to match.
* In order to accommodate IconList as a named component, Icons themselves needed to be refactored. All use of ReactUSWDS icons now follows the following syntax: <Icon.{IconName} /> instead of <Icon{IconName />. Furthermore, icons are no longer imported individually. Instead, Icon (the class) is imported to then use any <Icon.{IconName}> consumers require.

### Features

* add isInitiallyOpen option to modal ([#1971](#1971)) ([560564e](560564e))
* Make Summary Box more flexible ([#1929](#1929)) ([a46ed35](a46ed35))
* New Component: IconList ([#1691](#1691)) ([86589ac](86589ac))
* **ci:** add automerge priority label for Kodiak ([#1985](#1985)) ([9dc940e](9dc940e))


* Remove 1.x.x and 2.x.x deprecated properties ([#1988](#1988)) ([5dfadb1](5dfadb1))


### Documentation & Examples

* 404 page template added ([#2017](#2017)) ([0c9474d](0c9474d))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate headingLevel default values and make the prop mandatory
4 participants