-
Notifications
You must be signed in to change notification settings - Fork 125
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
[WIP] Entry theme selection #764
Conversation
spec/models/pageflow/theming_spec.rb
Outdated
config.themes.register(:named_theme) | ||
end | ||
|
||
theming = build(:theming, :theme_name => 'named_theme') |
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.
Use the new Ruby 1.9 hash syntax.
app/models/pageflow/revision.rb
Outdated
@@ -44,6 +44,10 @@ class Revision < ActiveRecord::Base | |||
validate :published_until_unchanged, :if => :published_until_was_in_past? | |||
validate :published_until_blank, :if => :published_at_blank? | |||
|
|||
validates_inclusion_of(:theme_name, in: lambda do |revision| | |||
Pageflow.config_for(revision.entry.account).themes.names | |||
end) |
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.
end at 49, 27 is not aligned with lambda do |revision| at 47, 44 or validates_inclusion_of(:theme_name, in: lambda do |revision| at 47, 4.
include RenderJsonHelper | ||
|
||
def themes_options_json_seed(config = Pageflow.config) | ||
config.themes.each_with_object({}) do |theme, options| |
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.
Prefer {...} over do...end for multi-line chained blocks.
} | ||
|
||
li.active { | ||
background-color: #d2e5fa; |
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.
Color literals like #d2e5fa
should only be used in variable declarations; they should be referred to via variable everywhere else.
margin: 10px; | ||
} | ||
|
||
li.active { |
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.
Merge rule li.active
with rule on line 77
} | ||
|
||
.use_theme { | ||
@include button(#590); |
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.
Color literals like #590
should only be used in variable declarations; they should be referred to via variable everywhere else.
overflow: auto; | ||
background-color: #fff; | ||
|
||
&>div { |
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.
Unnecessary parent selector (&)
bottom: 50px; | ||
left: 20px; | ||
overflow: auto; | ||
background-color: #fff; |
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.
Color literals like #fff
should only be used in variable declarations; they should be referred to via variable everywhere else.
right: 20px; | ||
max-width: 1100px; | ||
max-height: 650px; | ||
background-color: #eee; |
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.
Color literals like #eee
should only be used in variable declarations; they should be referred to via variable everywhere else.
@@ -0,0 +1,106 @@ | |||
button.change_theme { | |||
@include button(#559ff2); |
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.
Color literals like #559ff2
should only be used in variable declarations; they should be referred to via variable everywhere else.
4c23293
to
6217402
Compare
1 similar comment
6217402
to
190f533
Compare
190f533
to
c8e3a80
Compare
1 similar comment
- also move setting home_button_enabled to theming - rename copy_default_meta_tags to reflect this change and make it private - test the public API method instead of the now private helper
1 similar comment
e0b275a
to
0b8ee69
Compare
1 similar comment
d850dc8
to
9aa1c3e
Compare
1 similar comment
9aa1c3e
to
456ba9c
Compare
On choosing a page link, properly update title when it should be an empty string. Without this, e.g. in ReferenceInputView, `title` will not update in certain situations, because page#title() returns `'undefined'` due to `''` falsiness in Javascript. To reproduce: ``` this.configuration.get('title') === '' this.configuration.get('additional_title') === undefined ```
To be stored at `app/assets/images/pageflow/themes/$THEME_NAME/preview_thumbnail.png`.
Just from the code we cannot tell that there is a recommended way to deal with different attribute names.
509d196
to
efb03b7
Compare
1 similar comment
To be stored at `app/assets/images/pageflow/themes/$THEME_NAME/preview_thumbnail.png`.
1 similar comment
@@ -15,6 +15,8 @@ class ThemeGenerator < Rails::Generators::Base | |||
def copy_template | |||
directory('themes', File.join('app', 'assets', 'stylesheets', 'pageflow', 'themes')) | |||
empty_directory(File.join('app', 'assets', 'images', 'pageflow', 'themes', name)) | |||
copy_file('preview.png', "app/assets/images/pageflow/themes/#{name}/preview.png") | |||
copy_file('preview_thumbnail.png', "app/assets/images/pageflow/themes/#{name}/preview_thumbnail.png") |
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.
Line is too long. [109/100]
62c2cc6
to
bdbf888
Compare
1 similar comment
bdbf888
to
7e45b84
Compare
No description provided.