Breaking changes wishlist (for v16 & v17) #456
pretzelhammer
started this conversation in
Ideas
Replies: 1 comment
-
All except 2, 3, and 10 are addressed in this PR (v16): #500 We will address 2, 3, and maybe 10 in v17. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Normally I keep all of these things in my head but I thought I'd make it public and solicit suggestions from other Maker devs & Maker users:
Breaking changes I want to make eventually:
parentModal
frommodalApi.state.parentModal
. it's already been deprecated, we just need to remove it.@icons.chevronDown
and@icons.chevronUp
we should have@icons.selectClose = '@icons.chevronDown'
and@icons.selectOpen = '@icons.chevronUp'
and so on for the rest of the iconscritical
toerror
, e.g.@colors.critical
,@icons.critical
. it's weird that we generally useerror
andcritical
interchangeably in the codebase and public api and they're both mean exactly the same thing but a person can never be sure which to use in any particular situation without having to look it up first, which is annoying, so we should just get rid ofcritical
and useerror
everywhere.normal
&large
tomedium
&large
to be more consistent with the naming scheme for sizes we use across all other componentsclick-handler
from MMenuOption and use@click
insteadnull
values foriconName
in MProgressCircle we should make allundefined
&null
values illegal, and if people want to toggle the icon we should add ashow-icon
prop to MProgressCircle similar to how MToast workssize
prop instead of a Number pixel value/utils
to/components
and leave the/utils
directory purely for js helper functionsundefined
andnull
values should be made illegal for all themeable props (and for all props in general). This way merging nested subthemes behaves in a more simple and predictable manner. For example, let's say I have 2 props on a component, calledpropDoesntAllowNull
andpropAllowsNull
, and let's say this is show they're defined in the root parent theme of the app:So far so good, yet what happens when someone passes a subtheme object like this?:
The Maker theme resolver can't resolve these 2 props in a consistent uniform way, and trying to do so will break one or the other. For example:
null
is always a valid value, in which case it will passnull
topropAllowsNull
BUT it will also passnull
topropDoesntAllowNull
which will break itnull
is never a valid value, in which case it will pass'nonNullDefault'
topropAllowsNull
and'nonNullDefault'
topropDoesntAllowNull
so both will work, but maybe not in the way the developer intended, since it's possible the developer specifically WANTED the defaultnull
behavior forpropAllowsNull
since the prop does allownull
as a valid value and yet the theme resolver chose'nonNullDefault'
from the root parent theme insteadI hope it's clear that option 2 is better than option 1, but option 2 doesn't work reliably as long as well allow
null
orundefined
as valid values for themeable props, since it puts the theme resolver in the impossible position of having to read the developer's mind to know if they setnull
on a nullable prop because they want thenull
behavior or because they want to go further up the theming chain and inherit a value from the parent theme. TL;DR: we should disallownull
andundefined
as valid prop values. We can substitute any other arbitrary value if the default null behavior is complex, like the string'magic'
is need be.Breaking changes I maybe want to make:
v-model
for PinInput, remove thecomplete
event and refactor theshakeAndClearInputs
method to justshake
, or create a new MShake component that PinInput can be wrapped in and removeshakeAndClearInputs
entirely. I want to do this because PinInput is the only form component that doesn't havev-model
implemented, and the emitted event and public method were only implemented to compensate for the lack of av-model
, but the better solution would have been to just implementv-model
.Breaking changes I wanted to make but turns out I can't:
TransitionResize
as I thought it wasn't being used by anyone, but it turns out it's being used by the Checkout team, so it must remainBeta Was this translation helpful? Give feedback.
All reactions