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.

Sign up for GitHub

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

Jump to bottom

Remove line-height declaration #1881

Merged
merged 2 commits into from
Sep 17, 2017
Merged

Remove line-height declaration #1881

merged 2 commits into from
Sep 17, 2017

Conversation

julianxhokaxhiu
Copy link
Contributor

@julianxhokaxhiu julianxhokaxhiu commented Sep 16, 2017 •
edited by ivan-nginx
Loading

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?

Line height was set with a fixed size. This would break easily if you increase/decrease the base font-size.

before

What is the new behavior?

Removing the rule, allows the element to inherit the line-height, which is governed by the theme and auto-calculated. Making the overall pagination working just fine.

after

How to use?

Nothing to be done.

Does this PR introduce a breaking change?

  • Yes.
  • No.

@julianxhokaxhiu
Remove line-height declaration
5bf6c21 
It must not override the theme rule. Inherit is working just fine.
@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 16, 2017 •
edited
Loading

Then need to rewrite height of page-number. In 1st variant there are 29x31, but in second 29x33 and not see top border. Please, make changes cleaner.

U may do big pull request there will be all needed changes.

@julianxhokaxhiu
Copy link
Contributor Author

Sorry, I didn't understood. The change is just making the pagination behave exactly as it was before, when I do not set a custom font-size.

What is missing?

@ivan-nginx
Copy link
Collaborator

making the pagination behave exactly as it was before, when I do not set a custom font-size

No. it's not.
Please, create separate your own test site (subdoamin mb?) and check all changes there without any custom additions. U can check as i say above here » i unchecked in console line-height and boxes became taller.

@julianxhokaxhiu
Copy link
Contributor Author

That's just 2px difference. Assuming that this should be just 30px, does this mean that we have to fool around the CSS rules by enforcing a size in PX rather than just leaving the global line-height rule win overall?

To be honest I don't see why we should overengineer this. If it would have been 5-10px difference, that is noticeable. But from 31 to 29, well...better 2px less, but having a "naturally growing theme", than a complete workaround of calculations just to keep the box 30px in height ( which is not even true, by the way ).

If you don't feel happy with this PR, then I must consider using my own fork. But it's bad, as I always prefer to push changes that make sense to upstream. This one especially, is a real logic bug fix, not just "my wish to keep it as I like". I hope you can reconsider the fix as it is.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 16, 2017 •
edited
Loading

Can u chage this PR with normal height and width? Why so many words? It easy.

U say what pagination stay same when u not set any custom fonts, but it's not that. Yes, u do fix in line-height, but u brake for now styles also. I just talking about to make additionals without breaking another things, that's all — no matter 2px it is or repeating things. Can u make many changes at once and create 1 pull request with all possible fixes? Why u do so short pulls? I don't understand this, btw.

If you don't feel happy with this PR

I'm happy what u help to growing this theme. I also know what i can change any style i want in custom styles, for me it's not problem. But think, u do pulls in master branch and many users after update can be confused by breaking styles or other new things what they don't know. In this state, need to do short and global changes in dev branch and after some tests — merge dev into master. Or, yes, u can create your own fork, make changes all u want there, and after that (if u will want it) do big pull into master.

Take it easy, cool down. Not me also owner of this repo as u see. And i also have many local additions but i'm not merge them before tests and also trying to make breaking changes with solve all possible bugs on all schemes (but i use only Pisces/Gemini).

@julianxhokaxhiu
Copy link
Contributor Author

julianxhokaxhiu commented Sep 17, 2017 •
edited
Loading

Sorry, I don't get why should I introduce the width and height, when it is just fine. I can increase the padding if you want, which makes more sense. Is it fine if I do so?

Setting the padding as 1px 10px makes the box looking identical as before.

@ivan-nginx
Copy link
Collaborator

Padding or margin, no matter. If style of whole pagination box will look same, it's be ok. U can add styles directly in this pull.

In better way, pagination block must refactor in flexbox style with remove any table styles, but it's harder to do i think.

@ivan-nginx
Copy link
Collaborator

@julianxhokaxhiu ok, i now will refactor this styles. Do not touch this.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 17, 2017 •
edited
Loading

Within line-height before — 29x31 (desktop), 28x31 (mobile):
image
Without line-height after — 31x33 (desktop), 28x29 (mobile):
image

Responsinator: http://www.responsinator.com/?url=test.almostover.ru

@julianxhokaxhiu
Copy link
Contributor Author

Fine for me. Thanks! Can we close this PR? is the commit on master?

@ivan-nginx ivan-nginx merged commit 57641f6 into iissnan:master Sep 17, 2017
@ivan-nginx ivan-nginx added this to the v5.1.3 milestone Sep 17, 2017
@julianxhokaxhiu julianxhokaxhiu deleted the bugfix/pagination-line-height branch September 17, 2017 17:52
@julianxhokaxhiu
Copy link
Contributor Author

julianxhokaxhiu commented Sep 17, 2017 •
edited
Loading

I tested your patch and it seems that the border now is always below the long line. No matter which property I change. To be honest, it was much better before with that the property removed and that's it :\

broken

Any idea how can we fix it? Maybe it's just Mist related...

//EDIT: It seems that the property top: -1px was doing the trick. Restoring that line, makes the pagination working again as expected.

//EDIT2: Also, why padding: 0 11px? Didn't we agree on padding: 1px 10px in order to restore the height as before?

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 17, 2017 •
edited
Loading

Ok, top: -1px backed up.
padding: 0 11px for ~quadratic alignment without static line-height.
Please, tets paddings on Responsinator.

ivan-nginx added a commit that referenced this pull request Sep 17, 2017
@ivan-nginx
Fix to #1881.
1f85ef8
@julianxhokaxhiu
Copy link
Contributor Author

Gotcha :) thank you!

zqjimlove added a commit to zqjimlove/hexo-theme-next that referenced this pull request Sep 28, 2017
@zqjimlove
Merge branch 'source.master'
7f3d257 
* source.master: (1260 commits)
  [skip ci]
  Update PULL_REQUEST_TEMPLATE.md
  Update PULL_REQUEST_TEMPLATE.md
  Fix font-size only for mobile
  MOD: caution for Safari bug in `font` settings.
  Fix to iissnan#1881.
  Update pagination.styl
  Remove line-height declaration
  FIX: to iissnan#1829 global affix fix.
  Fix Mist font-size for Posts on mobile
  Update PULL_REQUEST_TEMPLATE.md
  Improve font configurations for global and headings
  Revert "FIX: Better way to adjust footer position in Pisces."
  Fix caption position
  Add Italian translation
  Fix fancybox style
  Uncomment `archives` after new option `override`.
  Update _config.yml
  Fix HoundCI issues
  Allow the user to override the whole configuration
  ...

# Conflicts:
#	_config.yml
wafer-li pushed a commit to wafer-li/hexo-theme-next that referenced this pull request Dec 29, 2017
@ivan-nginx @wafer-li
Fix to iissnan#1881.
fa3df0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v5.1.3
Development

Successfully merging this pull request may close these issues.
2 participants
@julianxhokaxhiu @ivan-nginx