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

Add blueDarkAlt theme deprecation notice for v6.0.0 #10

Merged
merged 3 commits into from
Sep 1, 2021

Conversation

daileytj
Copy link
Contributor

Fixes PXBLUE-2453.

Changes proposed in this Pull Request:

  • Add blueDarkAlt theme deprecation notice for v6.0.0

Additional Context

  • According to https://jsdoc.app/tags-deprecated.html, you can include some text that describes more about the deprecation, but I wasn't able to get that appearing in the IntelliSense popup.

This is what it looks like if you try to import the darkThemeAlt theme into a project.
Screen Shot 2021-08-30 at 3 24 44 PM

@jeffvg
Copy link
Contributor

jeffvg commented Aug 30, 2021

I've seen other depreciations also note the replacement of the named import. Should we do this as well?

@daileytj
Copy link
Contributor Author

daileytj commented Aug 30, 2021

I've seen other depreciations also note the replacement of the named import. Should we do this as well?

There's not really a "replacement" import per se. They would now just exclusively use the blueDark theme and potentially our new wrapper components, depending on the use case.

@joebochill
Copy link
Collaborator

So I assumed that the AltThemeProvider was in this repo, but it looks like it’s local to the auth workflow. We will probably want to add a deprecation warning there too.

Copy link
Collaborator

@joebochill joebochill left a comment

Choose a reason for hiding this comment

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

You may want to throw a console.warn in this file so that users will actually get a warning in the log (not just intellisense on hover). I think if you just drop it above the definition for the alt theme it should spit out the warning if anybody imports anything from that file.

@jeffvg
Copy link
Contributor

jeffvg commented Aug 30, 2021

I've seen other depreciations also note the replacement of the named import. Should we do this as well?

There's not really a "replacement" import per se. They would now just exclusively use the blueDark theme and potentially our new wrapper components, depending on the use case.

guess we could call out the updated import in the dep comment? use blueDark

Like the testing I was doing last week with Angular the 'get.testBed' was deprecated and it had a comment.. just an idea not a big deal

image

@jeffvg
Copy link
Contributor

jeffvg commented Aug 31, 2021

this is another example

image

add console warning
add alternative theme usage notice
@daileytj
Copy link
Contributor Author

I added the console.warn notice, but the additional jsdoc text is still not showing as an intellisense popup.

Simulator Screen Shot - iPhone 12 - 2021-08-31 at 08 54 32

@jeffvg
Copy link
Contributor

jeffvg commented Aug 31, 2021

yeah weird it doesn't show. I'm not familiar with this but is the @see still something that's used? I set it in my local d.ts file.

image

Do we have control on the quick fix link above? right now the quick fix is just remove the import (can it be change import to blueDark instead?

image

@daileytj
Copy link
Contributor Author

@jeffvg that @see thing isn't showing either. I can't see a way to configure the quick-fix options either, but that would be a nice feature. Is the deprecation message showing up for you? Just wondering if this is a local problem for me...

@jeffvg
Copy link
Contributor

jeffvg commented Aug 31, 2021

@jeffvg that @see thing isn't showing either. I can't see a way to configure the quick-fix options either, but that would be a nice feature. Is the deprecation message showing up for you? Just wondering if this is a local problem for me...

for me I see this when I hover over the striked through import name.. (the @see is here on my local)

image
image

@jeffvg
Copy link
Contributor

jeffvg commented Aug 31, 2021

ok this makes sense. Projects using ^5.1.0 of the theme would see the warning and depreciation info.

@daileytj daileytj merged commit bc5b104 into dev Sep 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/add-alt-dark-theme-deprecation-warning branch September 1, 2021 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants