COR-526: Featured Image Button #406

ElliottAYoung · 2016-12-14T21:21:14Z

Uses the changes from cortex-cms/cortex-plugins-core#22 to add the open popup button to the form. Tested locally and works when the core gem is even with Cortex

toastercup · 2016-12-14T22:17:10Z

@ElliottAYoung Reminder to follow PR naming conventions + attach labels + assign milestone

ElliottAYoung · 2016-12-14T22:18:22Z

@toastercup - shoot I thought I got this one, never hit save

toastercup · 2016-12-14T23:17:47Z

lib/tasks/employer/blog.rake

            ]
          },
+          {
+            "name": "Add Featured Image",


If you don't have anything to add for heading, remember that it's optional - we can remove it here

toastercup · 2016-12-14T23:19:38Z

app/assets/stylesheets/components/dialog.scss

  padding: 0;
 }
+
+.content_item_button {


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.

toastercup · 2016-12-14T23:24:11Z

app/assets/stylesheets/components/dialog.scss

+  padding-bottom: 20px;
+}
+
+.content_item_button-text {


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

toastercup · 2016-12-14T23:25:05Z

Gemfile

 gem 'acts-as-taggable-on', '~> 4.0'
 gem 'bcrypt', '~> 3.1.11'
-gem 'grape-kaminari', git: 'https://github.com/toastercup/grape-kaminari.git', branch: 'set-only-pagination-headers'
+gem 'grape-kaminari', git: 'https://github.com/toastercup/grape-kaminari.git', branch: 'master'


You'll want to switch grape-kaminari's branch back before a merge

toastercup · 2016-12-14T23:28:33Z

app/assets/javascripts/media_popups.js

   window.setModals()
 }
+
+$(".popup--open").on("click", function(ev){


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

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.

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

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

toastercup · 2016-12-14T23:29:13Z

app/assets/stylesheets/components/dialog.scss

+  text-align: center;
+}
+
+.logo {


What's this used for? It seems like an overly ambiguous name to be used in the global namespace

toastercup · 2016-12-14T23:29:32Z

app/assets/stylesheets/components/dialog.scss

+  }
+}
+
+.text-center {


If you're wanting to use re-usable classes such as these, make them mixins and include them in the semantic class that needs them

toastercup · 2016-12-14T23:30:17Z

app/assets/stylesheets/components/dialog.scss

+}
+
+.content_item_button-text {
+  font-size: 12px;


I believe this could be pulled from a variable that's already defined, but do we need to specify a different font size here?

the font sizes deviated a bit from the brand doc after Susan reviewed, so I'd make whatever directive you need if there's nothing that matches and add it to typography.scss with a comment to document how it's being used. Then we can look back at typography.scss and use it to update the brand doc. Start by trying to pick the closest thing from the brand doc though

toastercup · 2016-12-14T23:30:46Z

app/assets/stylesheets/components/dialog.scss

+}
+
+.button_content {
+  padding-top: 30px;


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

toastercup · 2016-12-14T23:32:26Z

app/assets/stylesheets/components/dialog.scss

+  margin-bottom: 10px;
+  width: 100%;
+  background-color: white;
+  color: #747D8E;


The colors contained in this class might also be defined in variables. I'm nitpicking here not because I care dearly for the styling/html/etc in V2, but because it could make our job easier when we have to port these overridden Material Design styles to the new React base - and also because @arelia spent some time recently to define these variables

what you need is some $color-anchor-blue-light

I also made a $color-white: white but i think that's kind of silly

arelia · 2016-12-15T00:24:54Z

app/assets/stylesheets/components/dialog.scss

+  color: #747D8E;
+
+  &:active {
+    background-color: #DDD;


$color-grey-lightest

arelia · 2016-12-15T00:25:55Z

app/assets/stylesheets/components/dialog.scss

+}
+
+.logo {
+  font-size: 56px;


maybe use the largest font style?

%display-text { // Cortex Logo color: $color-grey-dark; font-family: $cortex-font-stack; font-weight: normal; font-size: 2.1775rem; }

arelia · 2016-12-15T00:28:06Z

app/assets/stylesheets/components/dialog.scss

 }
+
+.content_item_button {
+  border: 1px solid #BBB;


$color-grey, also as you're adding color variables, update the comments next to the colors in colors.scss, at least for the greys. we have 5 shades of grey so far and I want to keep track of why

toastercup

This still needed to go through a last round of reviews, rather than a rush for verification

toastercup · 2016-12-15T20:12:20Z

Gemfile.lock


+GIT
+  remote: https://github.com/cortex-cms/cortex-plugins-core.git
+  revision: 889b620a1260f9b4412293812017c1f07e69f7cc


@ElliottAYoung this is still pointing to a branch

toastercup · 2016-12-15T20:13:22Z

app/assets/javascripts/media_popups.js

   window.setModals()
 }
+
+$(".popup--open").on("click", function(ev){


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

toastercup · 2016-12-15T20:14:37Z

lib/tasks/employer/blog.rake

+            "name": "Add Featured Image",
+            "description": "Add an image to feature with this Blog Post.",
+            "columns": [
+              {


Why does this say (optional heading)?

ElliottAYoung changed the title ~~Topic/cor 526 featured image button~~ COR-526: featured image button Dec 14, 2016

ElliottAYoung self-assigned this Dec 14, 2016

ElliottAYoung added the enhancement label Dec 14, 2016

ElliottAYoung added this to the Custom Content milestone Dec 14, 2016

ElliottAYoung requested review from toastercup, arelia and MKwenhua December 14, 2016 22:18

toastercup changed the title ~~COR-526: featured image button~~ COR-526: Featured Image Button Dec 14, 2016

toastercup requested changes Dec 14, 2016

View reviewed changes

arelia suggested changes Dec 15, 2016

View reviewed changes

ElliottAYoung added 4 commits December 15, 2016 09:32

Add Featured Image to Rake Task Seeder

9bc9587

Make JS modifications to properly trigger popup button

890f987

Change cursor to pointer on hover

41b4683

Add PR fixes to styles

3e4e7d4

ElliottAYoung force-pushed the topic/COR-526-Featured-Image-Button branch from 56f4ed4 to 3e4e7d4 Compare December 15, 2016 15:33

Gem and Styling fixes per Arelia

d2c85b4

arelia approved these changes Dec 15, 2016

View reviewed changes

ElliottAYoung merged commit 61dcae6 into develop Dec 15, 2016

ElliottAYoung deleted the topic/COR-526-Featured-Image-Button branch December 15, 2016 20:02

toastercup reviewed Dec 15, 2016

View reviewed changes

PlatformIntegrity unassigned ElliottAYoung Jun 4, 2017

COR-526: Featured Image Button #406

COR-526: Featured Image Button #406

Uh oh!

Conversation

ElliottAYoung commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toastercup commented Dec 14, 2016

Uh oh!

ElliottAYoung commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toastercup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ElliottAYoung commented Dec 14, 2016 •

edited

Loading