-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: page-list-upgrade
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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!
data/stylesheet.css
Outdated
@@ -1,3 +1,15 @@ | |||
.title-label { | |||
font-size:1em; | |||
/* font-size: 1.15em; |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/Widgets/PageItem.vala
Outdated
@@ -37,43 +37,45 @@ public class ENotes.PageItem : Gtk.ListBoxRow { | |||
private void build_ui () { | |||
set_activatable (true); | |||
|
|||
var margin_horizontal = 10; |
There was a problem hiding this comment.
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
…g and made translatable
…ot currently working)
…izontal margins to 12
ping. |
Fixes #258
Relies on #263
Requires running
cmake -DCMAKE_INSTALL_PREFIX=/usr ../
(in build directory)Changes made in this pull request:
Preview:
(the gap is fixed in another PR)