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

feat: Add Modal deprecation warnings (non-breaking deprecation) #825

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

brandonlenz
Copy link
Contributor

@brandonlenz brandonlenz commented Jan 26, 2021

Summary

Adds deprecation warnings to Modal components and functions/hooks in preparation for official removal from this library

Related Issues or PRs

closes #786

(I'm now realizing this issue was still under "Needs Requirements". I figured it'd be some low-hanging-fruit to get a warning out to consumers of this library as early as possible. Whenever we're ready, this branch exists to be merged (or updated then merged) pending any requirement changes)

How To Test

> git fetch
> git checkout bl-modal-deprecation-warning-786

> yarn storybook

Then preview any Modal story and take note of the console warnings (screenshots below as well)

Screenshots (optional)

Any single export will show a deprecation warning as follows:
image


Multiple exports will each show specific deprecation warnings
image


@trussworks-infra-zz
Copy link

trussworks-infra-zz commented Jan 26, 2021

Warnings
⚠️ This PR does not include changes to storybook, even though it affects component code.

Generated by 🚫 dangerJS against 3b7b689

Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

this looks great! do you mind updating the Modal tests to expect this warning? There are some examples of testing the deprecation warning in other components that use it, like NavList

@brandonlenz
Copy link
Contributor Author

this looks great! do you mind updating the Modal tests to expect this warning? There are some examples of testing the deprecation warning in other components that use it, like NavList

@suzubara good call. Thanks for pointing those out! d6194d3

Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

awesome, thank you!

@suzubara
Copy link
Contributor

the Happo report for the last commit has some weird diffs, like icons/fonts not showing up.. it might be worth trying to re-run to see if they were spurious

@brandonlenz
Copy link
Contributor Author

brandonlenz commented Jan 29, 2021

the Happo report for the last commit has some weird diffs, like icons/fonts not showing up.. it might be worth trying to re-run to see if they were spurious

@suzubara Looks like most of those went away after re-running.

That's really odd. The legitimate diffs with missing graphics appear to be in Edge. I noticed on the breadcrumb PR that the carets between breadcrumbs were rendering as squares in edge in Happo as well.

The accordion diff is weird. It looks like Happo is capturing screenshots while hovering over elements:
image

@suzubara
Copy link
Contributor

yeah that is weird, I haven't seen that before. I would chalk it up to Happo since the components themselves look fine

@brandonlenz brandonlenz merged commit 41d7e8e into main Feb 2, 2021
@brandonlenz brandonlenz deleted the bl-modal-deprecation-warning-786 branch February 2, 2021 23:17
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.

[feat] Deprecate the Modal component (non-breaking)
3 participants