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 #48 Footer position #550

Merged
merged 9 commits into from
Dec 31, 2018
Merged

Fix #48 Footer position #550

merged 9 commits into from
Dec 31, 2018

Conversation

stevenjoezhang
Copy link
Contributor

@stevenjoezhang stevenjoezhang commented Dec 30, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes have been added (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs have been added / updated (for bug fixes / features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

Issue Number(s): #48

https://galaxymimi.com/before/mist/
https://galaxymimi.com/before/muse/
https://galaxymimi.com/before/pisces/
https://galaxymimi.com/before/gemini/

What is the new behavior?

Description about this pull, in several words...

https://galaxymimi.com/after/mist/
https://galaxymimi.com/after/muse/
https://galaxymimi.com/after/pisces/
https://galaxymimi.com/after/gemini/

  • Screens with this changes:

Before & After
Muse:
2018-12-31 1 36 19
2018-12-31 1 43 26

Mist:
2018-12-31 1 36 48
2018-12-31 1 43 45

Pisces:
2018-12-31 1 37 03
2018-12-31 1 43 56

Gemini:
2018-12-31 1 37 20
2018-12-31 1 44 08

  • Link to demo site with this changes: N/A

How to use?

In NexT _config.yml:

...

Does this PR introduce a breaking change?

  • Yes.
  • No.

@ivan-nginx
Copy link
Member

Screens, please.

$div.remove();

return scrollbarWidth;
},

getContentVisibilityHeight: function() {
var docHeight = $('#content').height();
Copy link
Member

Choose a reason for hiding this comment

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

For what needed this changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$('#content').height() doesn't contain the height of #comments, and it will lead to a wrong calculation of the page's height

Copy link
Member

Choose a reason for hiding this comment

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

So, go see examples with getContentVisibilityHeight function. Because i remember, it was work fine (but mb not with all comments systems) and mb not now.

Copy link
Contributor Author

@stevenjoezhang stevenjoezhang Dec 30, 2018

Choose a reason for hiding this comment

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

You're right. I have thought about it, and seems it would be better to change it back... maybe it's just designed like this

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this function for rightly back2top percentage ratio. When comments systems exists, horizontal height changed dynamically when page will scrolled, so, b2t percent was counting not rightly.
This all i remember when add this function. For now some comment systems have dynamic loading (like lazy option for Disqus), so, i suggest to retest it in future.

Copy link
Member

@ivan-nginx ivan-nginx Dec 30, 2018

Choose a reason for hiding this comment

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

Here is changes for this function & b2t:

  1. iissnan/hexo-theme-next@4bd315d (PR #1574)
  2. iissnan/hexo-theme-next@028fe60
    and addition iissnan/hexo-theme-next@61086aa
    (PR #1898)

So, you can review this.

Copy link
Member

@ivan-nginx ivan-nginx Dec 30, 2018

Choose a reason for hiding this comment

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

$('#content').height() doesn't contain the height of #comments, and it will lead to a wrong calculation of the page's height

As we can see, b2t percent not contain comments div to counting to around wrong percentage ratio. And #content – meaning content of div > post-block, not content of comments block. That's why this function exists.

Copy link
Member

@ivan-nginx ivan-nginx Dec 30, 2018

Choose a reason for hiding this comment

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

Yes, need to discuss $('#content').height() and $('.container').height() in the future

Here is 2 problems:

  1. Comment system can be loaded with lazy (viewport height will more only when user scrolled down).
  2. Comment system loaded from 3rd-party side. So, JS on our server don't know which height we need to count.

But you can try to play around with this. Maybe you got success decision and make this coll feature (or bugfux).

Copy link
Contributor Author

@stevenjoezhang stevenjoezhang Dec 31, 2018

Choose a reason for hiding this comment

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

What about adding this to NexT v7.x Roadmap(#67)? I have reviewed the previous changes, but have no idea how to solve this...

Copy link
Member

@ivan-nginx ivan-nginx Dec 31, 2018

Choose a reason for hiding this comment

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

You can add it, of course. Anyway, I think NexT have more important things to improvment.

P.S. From next year we will start to work for v7.x and all bugfixes / features will pushed there. We also will resume maintenance for v6.x, but will be pushed only bugfixes. I want to create gantt roadmap something like in nodejs (https://github.com/nodejs/Release). Need to think about it better in details, any suggestions are welcome. In ideal need any chat with you to talk about this.

@stevenjoezhang
Copy link
Contributor Author

stevenjoezhang commented Dec 30, 2018

Screens, please.

Wait a minute

@ivan-nginx Screenshots added

@ivan-nginx
Copy link
Member

Also, read carefully this and this PR's.

Copy link
Member

@ivan-nginx ivan-nginx left a comment

Choose a reason for hiding this comment

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

Also, need to test on all possible resolutions all 4 schemes.
Here is good service for this: http://www.responsinator.com (or you can do it with browser if you want)
Screens or links, if it's possible.

@ivan-nginx
Copy link
Member

Bug in Pisces / Gemini:

Before:
image

After:
image


On Tablet & Mobile versions seems all fine. 👍

@stevenjoezhang
Copy link
Contributor Author

stevenjoezhang commented Dec 31, 2018

@ivan-nginx Fixed now
And the color of b2t icon seems wrong in Pisces & Gemini while setting b2t: false in sidebar

2018-12-31 1 56 47

2018-12-31 2 03 46

Copy link
Member

@ivan-nginx ivan-nginx left a comment

Choose a reason for hiding this comment

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

Well done!

@ivan-nginx ivan-nginx added this to the v6.7.0 milestone Dec 31, 2018
@stevenjoezhang stevenjoezhang merged commit 8a2051e into theme-next:master Dec 31, 2018
@stevenjoezhang stevenjoezhang changed the title Fix #48 Fix #48 Footer position Dec 31, 2018
tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
sslogan666 pushed a commit to sslogan666/sslogan666.github.io that referenced this pull request Jun 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants