Skip to content

COR-526: Featured Image Button #406

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

Merged
merged 5 commits into from
Dec 15, 2016
Merged

COR-526: Featured Image Button #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bc9587
Add Featured Image to Rake Task Seeder
ElliottAYoung Dec 14, 2016
890f987
Make JS modifications to properly trigger popup button
ElliottAYoung Dec 14, 2016
41b4683
Change cursor to pointer on hover
ElliottAYoung Dec 14, 2016
3e4e7d4
Add PR fixes to styles
ElliottAYoung Dec 15, 2016
d2c85b4
Gem and Styling fixes per Arelia
ElliottAYoung Dec 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,19 @@ GIT
awesome_nested_set (3.1.2)
activerecord (>= 4.0.0, < 5.1)

GIT
remote: https://github.com/cortex-cms/cortex-plugins-core.git
revision: 889b620a1260f9b4412293812017c1f07e69f7cc
branch: topic/COR-526-Popup-Button
Copy link
Member

Choose a reason for hiding this comment

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

@ElliottAYoung this is still pointing to a branch

specs:
cortex-plugins-core (0.4.6)
cells (~> 4.1)
cells-haml (~> 0.0.10)
cells-rails (~> 0.0.6)
ckeditor (~> 4.2.0)
mimemagic (~> 0.3.2)
rails (>= 4)

GIT
remote: https://github.com/samstickland/cells.git
revision: e5634e25b472d41913030eb82463b015f429f2ca
Expand Down Expand Up @@ -127,7 +140,7 @@ GEM
childprocess (0.5.9)
ffi (~> 1.0, >= 1.0.11)
choice (0.2.0)
ckeditor (4.2.0)
ckeditor (4.2.1)
cocaine
orm_adapter (~> 0.5.0)
climate_control (0.0.3)
Expand All @@ -143,13 +156,6 @@ GEM
concurrent-ruby (1.0.2)
connection_pool (2.2.0)
cortex-exceptions (0.0.4)
cortex-plugins-core (0.4.6)
cells (~> 4.1)
cells-haml (~> 0.0.10)
cells-rails (~> 0.0.6)
ckeditor (~> 4.2.0)
mimemagic (~> 0.3.2)
rails (>= 4)
database_cleaner (1.5.3)
debug_inspector (0.0.2)
descendants_tracker (0.0.4)
Expand Down Expand Up @@ -710,7 +716,7 @@ DEPENDENCIES
cells-rails (~> 0.0.6)
codeclimate-test-reporter (~> 0.6)
cortex-exceptions (= 0.0.4)
cortex-plugins-core (= 0.4.6)
cortex-plugins-core!
database_cleaner (~> 1.5)
devise (~> 4.2.0)
doorkeeper (~> 4.2)
Expand Down
5 changes: 5 additions & 0 deletions app/assets/javascripts/media_popups.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,8 @@ window.setModals = function () {
window.onload = function(){
window.setModals()
}

$(".popup--open").on("click", function(ev){
Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with the SASS living in Cortex, but this I feel should definitely live in the engine. Additionally, popup--open should be renamed to be specific to the featured button. Since the button is generically rendered, how will you determine which popup needs to be opened? I believe it should be passed as a config option at the field level, /or/ use the name of the field, underscored

Copy link
Member

Choose a reason for hiding this comment

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

Also valid: you could just spit out the name of the field in a data attribute on the popup button, then use that in this event callback to determine which popup to open. This is the simplest solution, I think. We could think harder about a better solution in V3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of right now the actual modal code only allows for one popup to ever exist - I agree that we should be differentiating between potential popups, but it won't actually do anything

Copy link
Member

Choose a reason for hiding this comment

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

There are 3 modals hardcoded with 3 different IDs, with 3 convenience methods for invoking each - not sure I'm following. This story is meant to iron out the logic for triggering the correct popup, so I don't know what story this bit would live in if not this one

ev.preventDefault();
window.MODALS.featured.open();
})
31 changes: 29 additions & 2 deletions app/assets/stylesheets/components/dialog.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
.dialog-div {
position: fixed;
background: #fff;
background: $color-white;
}

.dialog-backdrop {
Expand Down Expand Up @@ -37,9 +37,36 @@
bottom: 0;
right: 0;
left: 0;
background: white;
background: $color-white;
}

.mdl-dialog__content {
padding: 0;
}

.content_item_button {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this may be our first significant example of a plugin requiring styling. We can push this off for V2, but in V3, this styling must live in the plugin engine.

border: 1px solid $color-grey;
margin-top: 10px;
margin-bottom: 10px;
width: 100%;
background-color: $color-white;
color: $color-anchor-blue-light;
text-align: center;

&:active {
background-color: $color-grey-lightest;
}

&:hover {
cursor: pointer;
}
}

.button_content {
padding-top: 30px;
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend doin' some maths on the padding/margin sizing variables to derive the padding here

padding-bottom: 20px;
}

.content_item_button-text {
Copy link
Member

Choose a reason for hiding this comment

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

Not required for this PR, but noteworthy for the future: BEM would suggest you name dependent elements as such: content-item-button__text
Note the hyphens instead of underscores for multi-word element names, and the double-underscore for signifying a dependent element

@extend .text-style-7;
}
3 changes: 3 additions & 0 deletions app/cells/wizard/field/show.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@

- if field_item.field.field_type_instance.class == AssetFieldType
= cell(Plugins::Core::AssetCell, field_item, form: field_item_form, input_options: input, label_options: label).(:input)

- if field_item.field.field_type_instance.class == ContentItemFieldType
= cell(Plugins::Core::ContentItemCell, field_item, form: field_item_form).(:popup)
20 changes: 17 additions & 3 deletions lib/tasks/employer/blog.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ namespace :employer do
blog.fields.new(name: 'Categories', field_type: 'tree_field_type', metadata: {allowed_values: category_tree}, validations: {maximum: 2, minimum: 1})
blog.fields.new(name: 'Research', field_type: 'tree_field_type', metadata: {allowed_values: research_tree}, validations: {minimum: 1})
blog.fields.new(name: 'Persona', field_type: 'tree_field_type', metadata: {allowed_values: persona_tree})
blog.fields.new(name: 'Featured Image', field_type: 'content_item_field_type')

puts "Saving Employer Blog..."
blog.save
Expand Down Expand Up @@ -137,9 +138,6 @@ namespace :employer do
{
"id": blog.fields.find_by_name('Tags').id
},
# {
# "id": blog.fields.find_by_name('Expiration Date').id
# },
{
"id": blog.fields.find_by_name('Publish Date').id
}
Expand Down Expand Up @@ -196,6 +194,22 @@ namespace :employer do
}
]
},
{
"name": "Add Featured Image",
"description": "Add an image to feature with this Blog Post.",
"columns": [
{
"heading": "Image (Optional Heading)",
Copy link
Member

Choose a reason for hiding this comment

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

Why does this say (optional heading)?

"grid_width": 12,
"elements": [
{
"id": blog.fields.find_by_name('Featured Image').id,
"render_method": "popup"
}
]
}
]
},
{
"name": "Search",
"heading": "How can others find your post..",
Expand Down