Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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)

@aljelly aljelly requested a review from Philip-Scott as a code owner March 17, 2018 07:40
@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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants