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

Remove all hardcoded font sizes with replacing them on em #726

Closed
wants to merge 24 commits into from

Conversation

ivan-nginx
Copy link
Member

Resuming for #723 pull.

@ivan-nginx
Copy link
Member Author

Is there any work need to do?

Yes, there is.

@stevenjoezhang
Copy link
Contributor

I prefer to merge this PR, because it resolved 3 issues: #249, #687 and #629.
Using em instead of px is nice, should open another PR to do so

@ivan-nginx ivan-nginx self-assigned this Mar 30, 2019
@ivan-nginx ivan-nginx changed the title Removed all hardcoded font sizes with replacing them on em. 🚧 Removed all hardcoded font sizes with replacing them on em. Mar 30, 2019
@ivan-nginx ivan-nginx changed the title 🚧 Removed all hardcoded font sizes with replacing them on em. construction Removed all hardcoded font sizes with replacing them on em. Mar 30, 2019
@ivan-nginx ivan-nginx changed the title construction Removed all hardcoded font sizes with replacing them on em. WIP: Removed all hardcoded font sizes with replacing them on em. Mar 30, 2019
@stevenjoezhang
Copy link
Contributor

Removed all hardcoded font sizes is bugfix (for code font-size, and it's high priority)
with replacing them on emis feature & improvment (low priority)

As a bug fix, I think it is still necessary to be merged ASAP. Switch on em at once is not a good idea - I remember you have always told me to do only one thing in a single PR.

@1v9
Copy link
Member

1v9 commented Mar 31, 2019

Removed all hardcoded font sizes is bugfix (for code font-size, and it's high priority)
with replacing them on emis feature & improvment (low priority)

As a bug fix, I think it is still necessary to be merged ASAP. Switch on em at once is not a good idea - I remember you have always told me to do only one thing in a single PR.

Nice counterattack!😉

@ivan-nginx
Copy link
Member Author

Removed all hardcoded font sizes is bugfix (for code font-size, and it's high priority)
with replacing them on emis feature & improvment (low priority)

As a bug fix, I think it is still necessary to be merged ASAP. Switch on em at once is not a good idea - I remember you have always told me to do only one thing in a single PR.

I remember it too, yeah. But there is another situation: more things I've seen – breaked. It's actually most global refactoring, and move like this can't be only bugfix. If we start to refactor fonts, to remove hardcore styles, to change font sizes, etc., well, needed to make it beautiful. Making it at once and forgot about it...

As about talking mixed features / bugfixes / improvements: I will not edit in this PR button styles, fixes in swig or additions in js, u know? I'll do only what related to the Fonts, no less no more. And seems this PR not enter in tomorrow release, because there are many, many changes and need to carefully test it too. Important thing: don't hurry. Coz we already have a lot of changes in future v7.1.0 release.

@ivan-nginx
Copy link
Member Author

And actually this will be Improvement. Not Bug Fix and not Feature. Just Font will be better = Improvement.

@ivan-nginx ivan-nginx changed the title WIP: Removed all hardcoded font sizes with replacing them on em. Removed all hardcoded font sizes with replacing them on em. Mar 31, 2019
@ivan-nginx ivan-nginx changed the title Removed all hardcoded font sizes with replacing them on em. Removed all hardcoded font sizes with replacing them on em Mar 31, 2019
@ivan-nginx ivan-nginx changed the title Removed all hardcoded font sizes with replacing them on em Remove all hardcoded font sizes with replacing them on em Mar 31, 2019
@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Apr 1, 2019

@ivan-nginx So, is there any progress?
I have done all the jobs. Checkout this branch: https://github.com/stevenjoezhang/hexo-theme-next/tree/font

@stevenjoezhang stevenjoezhang self-assigned this Apr 1, 2019
@ivan-nginx
Copy link
Member Author

ivan-nginx commented Apr 1, 2019

Ok, I'll see it detailed later. For now as I see, seems it what NexT needed. Also, I don't find headers (H1-H6), I was implemented this function from Gemini (deleted lines in this PR). Don't hurry, we make this fonts ideal.

@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Apr 2, 2019

@ivan-nginx

I don't find headers (H1-H6)

They're still here, you can check again

I was implemented this function from Gemini (deleted lines in this PR)

No need to resume them. Why they were deleted? Because the target of this PR is Remove all hardcoded font sizes

You can push your changes/commits to font-refactoring branch, and I'll merge my commits, then let's do the further tests. Don't hurry, but can be more efficiency.

@stevenjoezhang
Copy link
Contributor

Besides, is this bot working properly?

屏幕快照 2019-04-02 下午6 50 24

@ivan-nginx
Copy link
Member Author

Besides, is this bot working properly?

屏幕快照 2019-04-02 下午6 50 24

WIP bot exists in this PR because he was installed during creation this PR. In new PR's all will be fine.

@stevenjoezhang
Copy link
Contributor

Superseded by #1005, #1006

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.

3 participants