Add deprecations for v6 #634
Conversation
vladucu
force-pushed
the
deprecate-action-argument
branch
from
f487b07
to
ddead3d
Compare
There was a problem hiding this comment.
Nice cleanup @vladucu! I would have preferred to see the class deprecation changes done as a separate PR as they're unrelated to the scope suggested by this PR's title. Perhaps we could update the title instead so that it more accurately reflects what's happening in this PR?
Wondering if it's worth introducing a decorator for the action deprecation - no strong views either way though.
Also worth checking the component documentation - we've so far specifically called out differences in param names, component usage etc. between the Ember and React implementations so we probably need to update the docs as part of this change.
| @@ -25,11 +25,10 @@ const avatarImages = [ | |||
|
|
|||
| const styleClasses = ['one', 'two', 'three', 'four', 'five', 'six']; | |||
|
|
|||
| @deprecateClassArgument | |||
There was a problem hiding this comment.
This is neat, but what do you think of having a generic deprecateArgument decorator instead so that we have something reusable going forward? Maybe gets a bit too clunky having to pass the various deprecation properties (message, version etc.)?
There was a problem hiding this comment.
yeah, I def like how simple the usage with this is
but something we should keep in mind and maybe do in the future 👍
| */ | ||
| secondaryAction = null; | ||
| } | ||
| export default class PolarisEmptyStateDetails extends Component {} |
There was a problem hiding this comment.
Do we still need this JS file? Also curious why the deprecation warning's been removed 🤔
There was a problem hiding this comment.
Seems still needed in addons?!
This is just internal, so no need to repeat the deprecations and documenting properties since we do in the parent component. Ideally, this would be just a tagless component
| @@ -14,7 +14,7 @@ | |||
| </div> | |||
| </PolarisTextContainer> | |||
|
|
|||
| {{#if action}} | |||
| {{#if @action}} | |||
There was a problem hiding this comment.
Should this be renamed as well? 🤔
vladucu
force-pushed
the
deprecate-action-argument
branch
from
ddead3d
to
a67bc8b
Compare
agree and sorry.... just trying to pick my battles 😂
I'd lean just leaving this as is, since it's just temporary code |
We've previously made a breaking change for
PolarisBannerwhere we renamedactionargumentprimaryAction.This adds backwards support for
actionargument too together with a deprecation. Same changes are included for all the other components that have been usingactionas an argument.Also, stops using a mixin for adding a deprecation re
classargument that was implicitly working previously with non-tagless components in Ember.