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

feat(tagcloud): new option class & level #4370

Merged
merged 4 commits into from
Jun 25, 2020
Merged

feat(tagcloud): new option class & level #4370

merged 4 commits into from
Jun 25, 2020

Conversation

stevenjoezhang
Copy link
Member

What does it do?

The color of the tag in the tag-cloud is defined by the inline style, e.g.

<a href="/tags/bcd/" style="font-size: 20px; color: #4682b4">bcd</a>

However, for themes that support the dark mode, it's necessary to display different tag colors for different background colors. Class name will be a better choice.
See also next-theme/hexo-theme-next@cbcd3f0

Usage:

{{ tagcloud({ class: 'tag-cloud' }) }}
for $tag-cloud in (0 .. 10) {
  $tag-cloud-color = mix($tag-cloud-end, $tag-cloud-start, $tag-cloud * 10);
  .tag-cloud-{$tag-cloud} {
    color: $tag-cloud-color;
  }
}

@media (prefers-color-scheme: dark) {
  for $tag-cloud in (0 .. 10) {
    $tag-cloud-color = mix($tag-cloud-end-dark, $tag-cloud-start-dark, $tag-cloud * 10);
    .tag-cloud-{$tag-cloud} {
      color: $tag-cloud-color;
    }
  }
}

How to test

git clone -b tagcloud https://github.com/hexojs/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@stevenjoezhang stevenjoezhang changed the title feat(tagcloud): new option class_name feat(tagcloud): new option class Jun 22, 2020
@@ -57,14 +58,15 @@ function tagcloudHelper(tags, options) {
const ratio = length ? sizes.indexOf(tag.length) / length : 0;
const size = min + ((max - min) * ratio);
let style = `font-size: ${parseFloat(size.toFixed(2))}${unit};`;
const name = className ? ` class="${className}-${Math.round(ratio * 10)}"` : '';
Copy link
Member

@jiangtj jiangtj Jun 23, 2020

Choose a reason for hiding this comment

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

I perfer ceil

if use round

Level 0 1 ... 9 10
Probability 5% 10% ... 10% 5%

if use ceil

Level 1 2 ... 9 10
Probability 10% 10% ... 10% 10%

Copy link
Member Author

Choose a reason for hiding this comment

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

But how about level 0? Math.ceil(0) == 0, but there is no level 0 in your Math.ceil table.
Your theory seems to describe the case where tag.length approaches infinity

Level 0 1 2 ... 9 10
Probability 0% 10% 10% ... 10% 10%

However, the actual situation is that tag.length is finite (and ratio is discrete), so the probability of level 0 must be greater than 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
const name = className ? ` class="${className}-${Math.round(ratio * 10)}"` : '';
let level = Math.ceil(ratio * 10);
if (level === 0) level ++;
const name = className ? ` class="${className}-${level}"` : '';

Copy link
Member Author

@stevenjoezhang stevenjoezhang Jun 23, 2020

Choose a reason for hiding this comment

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

In fact the asymmetry of level 0 and level 10 comes from this line

const ratio = length ? sizes.indexOf(tag.length) / length : 0;
const ratio = length ? (sizes.indexOf(tag.length) + 0.5) / length : 0;

Of course, I don't think this change is necessary. We are designing a blog system instead of some sort of scientific computing. Such an error (0.5 / length) is acceptable.

My conclusion is that there is no big difference between Math.ceil and Math.round in this situation.

Copy link
Member

@SukkaW SukkaW Jun 23, 2020

Choose a reason for hiding this comment

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

name could be replaced with attr.

Suggested change
const name = className ? ` class="${className}-${Math.round(ratio * 10)}"` : '';
const attr = className ? ` class="${className}-${Math.round(ratio * 10)}"` : '';

@coveralls
Copy link

coveralls commented Jun 23, 2020

Coverage Status

Coverage remained the same at 97.827% when pulling e6e587e on tagcloud into 2dfaa6a on master.

@stevenjoezhang stevenjoezhang requested a review from a team June 24, 2020 13:30
@stevenjoezhang stevenjoezhang changed the title feat(tagcloud): new option class feat(tagcloud): new option class & level Jun 24, 2020
Copy link
Member

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

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

LGTM!

@curbengh curbengh merged commit 7ff1a70 into master Jun 25, 2020
@curbengh curbengh deleted the tagcloud branch June 25, 2020 01:49
@SukkaW SukkaW mentioned this pull request Jul 16, 2020
22 tasks
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.

5 participants