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 Color class from tagcloud hexo helper #58

Merged
merged 4 commits into from
Aug 7, 2019
Merged

Add Color class from tagcloud hexo helper #58

merged 4 commits into from
Aug 7, 2019

Conversation

segayuu
Copy link
Contributor

@segayuu segayuu commented Jun 10, 2019

Agenda

tagcloud helper depends on the Color inner class. This class is reusable but is not currently tested alone.
Therefore, after porting to hexo-util, we separate tagcloud-dependent code from the original test. This improves the maintainability.

Clone

$ git clone https://github.com/segayuu/hexo-util
$ cd hexo-util
$ git fetch
$ git checkout Add-Color

Test

$ npm install
$ npm run test

Related

https://github.com/hexojs/hexo/blob/3.8.0/lib/plugins/helper/tagcloud.js
https://github.com/hexojs/hexo/blob/3.8.0/test/scripts/helpers/tagcloud.js

@segayuu segayuu requested a review from a team June 10, 2019 13:36
@tcrowe
Copy link

tcrowe commented Jun 10, 2019

That looks interesting, @segayuu! Is it possible you can add more information? Maybe also provide the code to clone & test?

@segayuu
Copy link
Contributor Author

segayuu commented Jun 11, 2019

Thank @tcrowe !

That looks interesting, @segayuu! Is it possible you can add more information? Maybe also provide the code to clone & test?

The PR text has been added (though it was originally 0 characters).

@yoshinorin
Copy link
Member

yoshinorin commented Jul 13, 2019

@tcrowe
It's just migrate from current hexo source code.

@segayuu
My understanding, we need following procedure. Is it correct?

  1. Merge this PR
  2. Publish new hexo-util
  3. Update hexo's hexo-util dependency & delete current hexo source code.
  4. Announce breaking change & create migration guide for users.

@yoshinorin
Copy link
Member

Btw, reported an issue about tagcloud hexojs/hexo#3622
We might be better to investigate it before merge this.

@segayuu
Copy link
Contributor Author

segayuu commented Jul 17, 2019

Thank @yoshinorin !
The procedure you suggested is basically correct, but only the fourth one is unnecessary.
The reason is that the Color class before PR is an inner class, is not exposed as a module, and has no way to access it (this is one of the purpose of PR).
From the user's point of view, it is not a Breaking Change, as it just added an API(it is necessary to fix it as a bug if its behavior changes).

Btw, reported an issue about tagcloud hexojs/hexo#3622
We might be better to investigate it before merge this.

I did not know the issue, thank you!
However, this is the code related to the tagcloud() and the Hexo class, not to the Color class itself.

@yoshinorin
Copy link
Member

yoshinorin commented Jul 30, 2019

@segayuu

However, this is the code related to the tagcloud() and the Hexo class, not to the Color class itself.

OK. Understood 😃

IMHO we might as well include this PR to hexo v4. How do you think?
If yes, we should review & merge this PR & release a new version of hexo-util before release hexo v4.

hexojs/hexo#3508

@segayuu
Copy link
Contributor Author

segayuu commented Jul 30, 2019

@yoshinorin I agree with your opinion! 😆

tomap
tomap previously approved these changes Aug 7, 2019
Copy link
Contributor

@tomap tomap left a comment

Choose a reason for hiding this comment

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

LGTM

@tomap
Copy link
Contributor

tomap commented Aug 7, 2019

@hexojs/core anyone wants to add something or we can merge?

@curbengh
Copy link
Contributor

curbengh commented Aug 7, 2019

I suggest to rebase to include test in node 8-12.

@segayuu
Copy link
Contributor Author

segayuu commented Aug 7, 2019

@curbengh Since rebase was done and a new error was corrected, there was no problem with merge.

@segayuu segayuu merged commit 52820ac into hexojs:master Aug 7, 2019
@segayuu segayuu deleted the Add-Color branch August 7, 2019 14:03
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 95.214% when pulling 6d9df36 on segayuu:Add-Color into ab30d63 on hexojs:master.

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.

6 participants