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

Improve font configurations for global and headings #1879

Merged
merged 1 commit into from
Sep 16, 2017
Merged

Improve font configurations for global and headings #1879

merged 1 commit into from
Sep 16, 2017

Conversation

julianxhokaxhiu
Copy link
Contributor

@julianxhokaxhiu julianxhokaxhiu commented Sep 16, 2017 •
edited
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).
  • 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?

At the current status, generic font-sizes can't be configured by just editing the .yml configuration file. A fork of the theme is required, and an edit of some files is required.

What is the new behavior?

The new approach goes towards an easier configuration of font-size throught the yml file:

  • Add a possibility to configure them via the _config.yml file ( or data file )
  • Reflect the configuration in base.styl
  • Remove hardcoded font-sizes for headings inside post-title.styl
  • Improve documentation around font customization inside the _config.yml file

How to use?

In NexT _config.yml:
See _config.yml inside this PR

Does this PR introduce a breaking change?

  • Yes.
  • No.

@julianxhokaxhiu
Improve font configurations for global and headings
a27ed11 
-> Add a possibility to configure them via the _config.yml file ( or data file )
-> Reflect the configuration in base.styl
-> Remove hardcoded font-sizes for headings inside post-title.styl
-> Improve documentation around font customization inside the _config.yml file
@ivan-nginx
Copy link
Collaborator

What about if user want to define not in px but in em?

ivan-nginx
ivan-nginx reviewed Sep 16, 2017
View reviewed changes
source/css/_variables/base.styl
@@ -92,6 +92,7 @@ $font-weight-bolder = 700

// Font size
$font-size-base = 14px
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems duplicate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the style of the other declaration, seems a safe "default" way in stylus. Isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, but i think need to set 14px in config by default instead of reading $font-size-base var twice.

font:
  global:
    size: 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I will create another PR then to get rid of this double declarations. I saw a lot of them during the patch making.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hope u know what u are doing.
Also, always remember what in NexT is 4 schemes. And if on one of them (for ex on Mist) worked well, on another scheme this can be broken. Muse and Mist have aproach structure, but Pisces is very difference from them. Gemini is mirror of Pisces with some style remakes and additions. So, please, make the tests at least on two schemes (Muse || Mist && Pisces || Gemini).

ivan-nginx
ivan-nginx reviewed Sep 16, 2017
View reviewed changes
source/css/_common/components/post/post-title.styl
@@ -1,12 +1,7 @@
.posts-expand .post-title {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to define variables here like font-size: $font-size-base;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is inherited from the global h1...h6 declaration in the scaffolding part. While this one was just overriding it always with a fixed px value.

@julianxhokaxhiu
Copy link
Contributor Author

Related to the em: I'm totally open to it. I just followed the existing piece of configurations in the _config.yml. I'm totally open to support it, but will just differ from the goal of this single PR.

@ivan-nginx
Copy link
Collaborator

Yep, need to think about em and px sizes selection. Because, mobiles and other devices for now became with different resolutions and NexT support mobile style adaptivity.

@julianxhokaxhiu
Copy link
Contributor Author

Totally agree! Thanks for the comprehension :)

@ivan-nginx ivan-nginx added this to the v5.1.3 milestone Sep 16, 2017
@ivan-nginx ivan-nginx merged commit c51f0ef into iissnan:master Sep 16, 2017
@julianxhokaxhiu julianxhokaxhiu deleted the feature/font-configuration branch September 16, 2017 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivan-nginx ivan-nginx ivan-nginx left review comments

Assignees
No one assigned
Labels
Discussion
Projects
None yet
Milestone
v5.1.3
Development

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