Skip to content

Add Color class from tagcloud hexo helper#58

Merged
segayuu merged 4 commits intohexojs:masterfrom
segayuu:Add-Color
Aug 7, 2019
Merged

Add Color class from tagcloud hexo helper#58
segayuu merged 4 commits intohexojs:masterfrom
segayuu:Add-Color

Conversation

@segayuu
Copy link
Contributor

@segayuu segayuu commented Jun 10, 2019 •
edited
Loading

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 •
edited
Loading

@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

yoshinorin commented Jul 13, 2019

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 •
edited
Loading

@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! 😆

@yoshinorin yoshinorin mentioned this pull request Aug 3, 2019
Merged
tomap
tomap previously approved these changes Aug 7, 2019
View reviewed changes
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 •
edited
Loading

@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
@yoshinorin yoshinorin mentioned this pull request Aug 7, 2019
Closed
@coveralls
Copy link

coveralls commented Aug 7, 2019

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

Reviewers

@yoshinorin yoshinorin yoshinorin approved these changes

@tomap tomap tomap left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@segayuu @tcrowe @yoshinorin @tomap @curbengh @coveralls