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

Deprecate TipBox #2121

Merged
merged 5 commits into from
Feb 6, 2023
Merged

Deprecate TipBox #2121

merged 5 commits into from
Feb 6, 2023

Conversation

itsyme
Copy link
Contributor

@itsyme itsyme commented Jan 30, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Resolves #1804. Changes all appropriate instances of tipbox to box.

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Deprecate TipBox

Both tip-box and box are supported with box being an alias for
tipbox.

This could be confusing for users and developers as the use of
tip-box is not documented.

Let's deprecate tipbox, remove support for the tip-box tag and
replace all instances of tipbox to box.

This prevents confusion between the use of variations of tipbox in
both code and documentation, making any reference to or use of this
this component consistent. Replacing tipbox with box removes the
need for box being an alias for tipbox, deprecating tipbox completely.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

I found some other instances of Tip Box which I'm not sure if should be renamed as well.

packages/vue-components/src/tests/TipBox.spec.js

packages/vue-components/src/utils/utils.js
Comment // Used in the TipBox component to classify the different styles used by bootstrap from the user input.

@itsyme
Copy link
Contributor Author

itsyme commented Jan 30, 2023

I found some other instances of Tip Box which I'm not sure if should be renamed as well.

packages/vue-components/src/tests/TipBox.spec.js

packages/vue-components/src/utils/utils.js Comment // Used in the TipBox component to classify the different styles used by bootstrap from the user input.

Hi @yucheng11122017! Thanks for looking through my PR, I intentionally left these as I felt they were referring to the TipBox component which is still currently named as TipBox.vue under vue-components. I am currently still unsure about whether to change this and would be open to discussion about this!

@lhw-1
Copy link
Contributor

lhw-1 commented Jan 31, 2023

I intentionally left these as I felt they were referring to the TipBox component which is still currently named as TipBox.vue under vue-components. I am currently still unsure about whether to change this and would be open to discussion about this!

If we're going by the original intent of fully deprecating the tip-box component as the box component, then I think we can rename these as well. Referring to all instances of tip-box as box in both development and documentation should make things easier!

Of course, the other concern would be backwards compatibility, so while we can make box as the main component, perhaps any usage of tip-box should still be supported (as an alias of box or otherwise)?

@jovyntls
Copy link
Contributor

Of course, the other concern would be backwards compatibility, so while we can make box as the main component, perhaps any usage of tip-box should still be supported (as an alias of box or otherwise)?

We shouldn't need to support any usage as an alias since it's been undocumented since quite a while back (see this comment). Removing it altogether is fine :)

As for renaming tipBox.vue -> Box.vue, I'm fine with either way - I originally thought using Box.vue is a common name and could cause naming conflicts in the future but upon further thought I don't think this is so (and even if it does it shouldn't be too big of a problem).

@itsyme
Copy link
Contributor Author

itsyme commented Jan 31, 2023

Alright! I will rename all variants of tipbox to box and rename the component in the next commit!

Copy link
Contributor

@jovyntls jovyntls left a comment

Choose a reason for hiding this comment

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

LGTM! 🎊 Thanks @itsyme , can we also add more details to the commit message since this is a (improbable, but possibly) breaking deprecation?

@jovyntls jovyntls added this to the 4.0.3 milestone Feb 1, 2023
@itsyme
Copy link
Contributor Author

itsyme commented Feb 1, 2023

Hi @jovyntls I have edited the commit message! Let me know if you would like any additions or changes to it.

@tlylt
Copy link
Contributor

tlylt commented Feb 1, 2023

Hi @jovyntls I have edited the commit message! Let me know if you would like any additions or changes to it.

Hi @itsyme, I think @jovyntls (and I too) would think that a more detailed commit message following the following structure (as noted in the link in #2121 (review)) would be more suitable, could you update the commit message accordinly to it?

{current situation} -- use present tense

{why it needs to change}

{what is being done about it} -- use imperative mood

{why it is done that way}

{any other relevant info}

@itsyme
Copy link
Contributor Author

itsyme commented Feb 1, 2023

Hi @tlylt! Thank you for the clarification and the suggested structure! I interpreted it as a more detailed commit header. I will make the relevant changes now!

@jovyntls
Copy link
Contributor

jovyntls commented Feb 2, 2023

Thanks for making the changes @itsyme 😄
I see that you're following the scope: change commit message guidelines for the commit header. Can we go back to the non-scope format (e.g. Deprecate TipBox) for the commit header? This is to be consistent with the rest of the commit messages and because the scope of tip-box tag and TipBox is relatively small. The differentiation between the tag and component can be detailed in the message body, if needed

Copy link
Contributor

@jovyntls jovyntls left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the changes @itsyme 😄

@jovyntls jovyntls changed the title Deprecate tipbox Deprecate TipBox Feb 6, 2023
@jovyntls jovyntls merged commit 56a3510 into MarkBind:master Feb 6, 2023
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.

tip-box is not documented and can be deprecated
5 participants