markbind icon indicating copy to clipboard operation
markbind copied to clipboard

Deprecate add-class

Open tlylt opened this issue 3 years ago • 6 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Any related issues?

#1280

What is the area that this feature belongs to?

Syntax

Is your feature request related to a problem? Please describe.

Raised in https://github.com/MarkBind/markbind/issues/1280#issuecomment-660620171

on add-class, actually, was there a reason for introducing this and not just using class? (vue automatically forwards class to the root element of the component) If not maybe we could deprecate that by v3.0 as well

Describe the solution you'd like

If it's fine to just use class, perhaps we can deprecate this "add-class" feature.

Usage of add-class in our UG will have to be modified as well.

Describe alternatives you've considered

No response

Additional context

I am not too sure what is the implementation that supports this "add-class" feature...after doing a brief search for "add-class" in the source code.

tlylt avatar Apr 11 '22 09:04 tlylt

This might have been a leftover from vue v1->v2 migration.

Agreed on standardising to class as well, but some custom implementations may be needed (e.g. for modals) for components where class should not be applied to the root element.

ang-zeyu avatar Apr 18 '22 05:04 ang-zeyu

I think add-class was meant as 'append this to the existing value of the class attribute (as opposed to replace the existing value with the new value). Is there any need to differentiate between these two, or class behavior is already append (not replace)?

damithc avatar Apr 18 '22 09:04 damithc

I think add-class was meant as 'append this to the existing value of the class attribute (as opposed to replace the existing value with the new value). Is there any need to differentiate between these two, or class behavior is already append (not replace)?

I am ok with retaining both for backward compatibility and since add-class is documented.

I am unable to find any custom implementation that we did to handle "add-class", @ang-zeyu do you know if it is a feature of a library that we imported (Vue?)? (for curiosity's sake... I can't find anything related to "add-class" on Google 😢).

tlylt avatar Aug 02 '22 05:08 tlylt

class behavior is already append

It's already append. (iirc, to be verified*)

I am unable to find any custom implementation that we did to handle "add-class"

I think it might have been removed in some PR since my comment, can't find it either too.

There's one I'm not sure about in Question.vue, if it forwards properly then all good. 🙂 Other than that let's also make sure it forwards to all the v-else / v-else-if cases correctly.

it is a feature of a library that we imported (Vue?)

I don't have much context here either. (whether it was VueStrap's or our implementation) But surely it is something custom; Since its now forwarded automatically I don't think there's any reason to keep using addClass as well.

But you can dig through here if you're interested and have the time.

Would suggest:

  • implement some build-time warnings for addClass in v4.1 / 4.0.1 to deprecate this
  • then completely remove it in v5

ang-zeyu avatar Aug 06 '22 06:08 ang-zeyu

Bumping this issue as we're nearing a v5 release! We'll be able to merge breaking changes for the upcoming release since it's a major version increment, so now would be a good time if any large changes are needed :)

jovyntls avatar Mar 23 '23 15:03 jovyntls

The impact of this is quite minimal...we can push this to v6.

tlylt avatar Jul 16 '23 03:07 tlylt