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

fix: compatibility with existing theme #114

Merged
merged 4 commits into from Nov 27, 2019
Merged

Conversation

@curbengh
Copy link
Contributor

curbengh commented Nov 22, 2019

Fixes #112

Existing theme expects feed.path to be a string, but previous PR #96 converts it to array. This PR prevents the conversion.

cc @mikolaje @yrpang @AlynxZhou @stevenjoezhang

@curbengh curbengh requested a review from hexojs/core Nov 22, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 22, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 0314e98 on curbengh:theme-compat into 9f1f7b0 on hexojs:master.

@SukkaW

This comment has been minimized.

Copy link
Member

SukkaW commented Nov 25, 2019

Should we fix #115 in this PR as well? We could create a ctx instead of writing new syntax of config back to hexo.

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Nov 25, 2019

This PR also fixes #115.

This plugin needs to write back to user config to ensure consistency. If it is not consistent, it can break this feature especially this line.

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Nov 26, 2019

Whether or not this plugin should write back to user config actually doesn't matter, because a theme still need

    <% if (config.feed) { %>
      <% if (config.feed.type.length && config.feed.path.length) { %>
        <% if (typeof config.feed.type === 'string' )) { %>
          <link rel="alternate" type="application/<%- config.feed.type.replace(/2$/, '') %>+xml" title="<%= config.title %>" href="<%- url_for(config.feed.path) %>">
        <% } else { %>
          <% for (const i in config.feed.type) { %>
            <link rel="alternate" type="application/<%- config.feed.type[i].replace(/2$/, '') %>+xml" title="<%= config.title %>" href="<%- url_for(config.feed.path[i]) %>">
          <% } %>
        <% } %>
      <% } %>
    <% } %>

to be compatible with both #100 (released in 2.1) and older versions of this plugin.

@stevenjoezhang

This comment has been minimized.

Copy link

stevenjoezhang commented Nov 26, 2019

We have noticed this problem about the type of feed.path: theme-next/hexo-theme-next@c78f3ab
This Pull Request will be very helpful, thanks!

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Nov 26, 2019

We have noticed this problem about the type of feed.path: theme-next/hexo-theme-next@c78f3ab

cc @mikolaje


@stevenjoezhang
But theme-next/hexo-theme-next@c78f3ab is still not compatible with #100 though.

I also noticed the theme doesn't support RSS2 format. Relevant PR yscoder/hexo-theme-indigo#489.

@stevenjoezhang

This comment has been minimized.

Copy link

stevenjoezhang commented Nov 26, 2019

I will do some research. Compatibility with these core plugins of Hexo is critical to a theme.

@curbengh curbengh requested review from hexojs/core and removed request for hexojs/core Nov 27, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Nov 27, 2019

@hexojs/core this is an urgent fix.

@SukkaW
SukkaW approved these changes Nov 27, 2019
@curbengh curbengh merged commit 6e5bb89 into hexojs:master Nov 27, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@curbengh curbengh deleted the curbengh:theme-compat branch Nov 27, 2019
@curbengh curbengh mentioned this pull request Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.