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

[Relies on #263] Style update #264

Open
wants to merge 15 commits into
base: page-list-upgrade
Choose a base branch
from

Conversation

aljelly
Copy link

@aljelly aljelly commented Mar 17, 2018

Fixes #258

Relies on #263

Requires running cmake -DCMAKE_INSTALL_PREFIX=/usr ../ (in build directory)

Changes made in this pull request:

  • Add stylesheet
  • Modify app styling
  • Change default editor font size (to 10)

Preview:

image

(the gap is fixed in another PR)

@Philip-Scott Philip-Scott added this to the PageList upgrade milestone Mar 17, 2018
Copy link
Owner

@Philip-Scott Philip-Scott left a comment

Choose a reason for hiding this comment

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

Hey! Thanks again for helping me make Notes-Up even better!

Like you said, this branch relies on your other PR which i've already reviewed, so you should check that first ;) But i also left a couple of reviews in this PR

If you have any questions feel free to leave a comment and reach out to me!

@@ -1,3 +1,15 @@
.title-label {
font-size:1em;
/* font-size: 1.15em;
Copy link
Owner

Choose a reason for hiding this comment

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

Don't leave commented code :)

@@ -91,7 +91,7 @@
</key>

<key name="editor-font" type="s">
<default>"Open Sans 12"</default>
<default>"Open Sans 10"</default>
Copy link
Owner

Choose a reason for hiding this comment

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

Why the change to 10?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it was more in line with how fonts on the system are sized, I can change it back if you don't like it/don't think it's good.

@@ -37,43 +37,45 @@ public class ENotes.PageItem : Gtk.ListBoxRow {
private void build_ui () {
set_activatable (true);

var margin_horizontal = 10;
Copy link
Owner

Choose a reason for hiding this comment

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

  • If we're going to use a constant, you should declare it at the very top of the class as private const int HORIZONTAL_MARGIN = 10
  • Why the 10? why not something like 6 or 12? (12 is actually the number recommended over at the elementary Human Interface Guidelines: https://elementary.io/docs/human-interface-guidelines#spacing

@Philip-Scott Philip-Scott changed the base branch from master to page-list-upgrade March 17, 2018 15:51
@snowparrot
Copy link
Contributor

ping.

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