[DEV APPROVED] 7938 resize promo article images #1691

Tr4pSt3R · 2017-03-01T13:04:33Z

The main work on the CMS side to enable this feature can be seen here: moneyadviceservice/cms#383

This change is

norm · 2017-03-01T13:19:32Z

This PR as it stands is a 👎 from me. Whilst the end result (as seen in the "Files changed" tab) looks good, the narrative that plays out in each commit is not. There's code and images that get added and then removed again (which is fine during dev, but should be rebased out before issuing a PR), and the individual commits could do with better explanations.

I think it needs to be reworked so that it is one or two good commits. If you're not sure how, I'm happy to pair with you to make it better.

rnalexander · 2017-03-01T13:26:54Z

Please to not merge until have you've had a chance to pair with Norm on how to improve the structure.

Tr4pSt3R · 2017-03-03T09:41:49Z

♻️
@norm @rnalexander

norm

What's with the asterisks in the commit message?

norm · 2017-03-03T09:56:18Z

lib/core/repository/cms/attribute_builder.rb


-        attributes
+        hsh


What is the purpose of renaming this?

@norm This is to avoid overloading the method name. I will change it to something more meaningful

tomas-stefano · 2017-03-06T16:07:34Z

app/views/shared/_article_promos.html.erb

-            <%= image_tag(item['image'], alt: '', class: 'promo__img') %>
+            <%= image_tag(item['image'],
+              alt: '',
+              sizes: '(max-width: 479px) 100vw, (max-width: 959px) 50vw, 25vw',


Is the view the best place to define this sizes attribute?

@tomas-stefano it's used nowhere else but here. Where is the best place to put this or what is the best practice on handling this, please?

Ok. Is this line very important for the whole feature? What happen with the functionality if I remove this line?

@tomas-stefano If that line is removed the srcset will not work at the viewports which we expect them to be activated.

So, if you remove this line ... will the tests fail to indicate that the feature is not working properly?

The tests will not fail neither will there be any catastrophic failure. Are you advocating the addition of a test?

I am not advocating anything. In order to give you the info on the best place to put this line, I need to understand all the implications of it. That's the point of my questions. But I didn't understand one thing: you said that if this line is removed the srcset will not work at the viewports and in the other you said there will not be any catastrophic failure. Could not be catastrophic, but is still a failure and the functionality will not work as expected ... right?

tomas-stefano · 2017-03-06T16:08:52Z

spec/lib/core/repository/cms/attribute_builder_spec.rb

+    end
+
+    describe '.translate_attributes_from_raw_blocks' do
+      let(:hsh) do


Can it be renamed to hash, please? IMHO abbreviation it is only useful when it is very very long word.

Actually, it can be renamed by blocks attributes or even page content itself. Or event the response body (if it is the faked response from CMS)

tomas-stefano · 2017-03-06T16:09:05Z

spec/lib/core/repository/cms/attribute_builder_spec.rb

+    end
+
+    describe '.group_nested_attributes' do
+      let(:hsh) do


Can it be renamed to hash, please? IMHO abbreviation it is only useful when it is very very long word.

Actually, it can be renamed by blocks attributes or even page content itself.

tomas-stefano · 2017-03-06T16:10:53Z

lib/core/repository/cms/attribute_builder.rb

@@ -12,23 +12,21 @@ def self.build(response)
      end

      def attributes
-        attributes = response.body
+        attrs = response.body


Is there a reason to abbreviate to attrs?

@tomas-stefano it is abbreviated to attrs as the next best alternative attributes is already used as the method name. Due to how it's heavily used by other methods, it's best to keep the sentiment of attributes even if it's in an abbreviated form.

30 def set_title_from_label(attributes) 31 attributes['title'] = attributes['label'] 32 end 33 34 def set_body_from_content_block(attributes) 35 attributes['body'] = BlockComposer.new(attributes['blocks']).to_html 36 end 37 38 def set_description(attributes) 39 attributes['description'] = attributes['meta_description'] 40 end 41 42 def set_categories(attributes) 43 attributes['categories'] = attributes['category_names'] 44 end

Ok, I understand that is heavily used, but it is mutating the same object, so the names could be the same without any problem. I would return to attributes as before because the code becomes better to read.

@tomas-stefano I did something like this and @norm had other ideas. I can see why both your ideas are viable. I need a second opinion on this. I am torn between whether to overload the method name or not.

@jriga wdyt, please?

@Tr4pSt3R I think you're confusing your notes from our pairing session. My comment on not overloading variable names was based on a piece of code where you reused a variable to contain two different values (that I don't like). This was fine.

I just wanted to say I'm so proud of all of you right now. 😍

Thanks for clarifying @norm.
Thanks for pointing that out @tomas-stefano.

tomas-stefano · 2017-03-06T16:12:16Z

spec/lib/core/repository/cms/attribute_builder_spec.rb

@@ -1,7 +1,87 @@
+# coding: utf-8


Is this line really need it?

tomas-stefano · 2017-03-06T16:12:36Z

spec/lib/core/repository/cms/attribute_builder_spec.rb

 module Core::Repository::CMS
  RSpec.describe AttributeBuilder do
    let(:body) { File.read('spec/fixtures/cms/beginners-guide-to-managing-your-money.json') }
    let(:response) { OpenStruct.new(body: JSON.parse(body)) }
+    let(:ab)  { AttributeBuilder.new(:resource) }


Can we rename to attribute_builder, please?

tomas-stefano · 2017-03-06T16:13:29Z

spec/lib/core/repository/cms/attribute_builder_spec.rb

+    let(:ab)  { AttributeBuilder.new(:resource) }
+
+    describe '.set_title_from_label' do
+      let(:hsh) { { 'label' => :foo } }


Is this hsh variable the response body? If so, can we rename to response_body?

pmcostello · 2017-03-09T17:56:20Z

why was the gem rb-readline being added then removed in the last two commits? also could do with a bit more detail in the commit message

Tr4pSt3R · 2017-03-10T08:28:26Z

@pmcostello it's a fixup commit so git automatically adds a commit message to indicate that it's a temporary commit.

rb-readline was added because the system readline library that the Pry debugger uses is mishaving. Rb-readline is a pure Ruby implementation of the readline library. It was temporary to jump into the console to inspect some objects during runtime.

There will no trace of it in the final commit.

Tr4pSt3R · 2017-03-10T08:29:05Z

♻️ @norm @pmcostello @rnalexander @jriga @davidtrussler

jriga · 2017-03-10T10:46:04Z

looks good to me, just needs git rebase -i --autosquash to be ready

pmcostello · 2017-03-10T11:47:55Z

ok thanks for the explanation @Tr4pSt3R, squash and merge 👍

Tr4pSt3R · 2017-03-10T16:10:29Z

♻️ @jriga @pmcostello

jriga · 2017-03-10T16:17:44Z

👍

tomas-stefano · 2017-03-14T17:19:10Z

I don't want to sound picky, but please correct if am wrong. Since my understanding of the code that I read:

the site makes a request to the CMS and it is expecting on the json response to have the srcset attribute for every image.
The site renders the img tag adding a srcset attribute and a sizes attribute to the img tag.

Correct if I am wrong but without any of these, the whole feature will not work properly. RIght?

Conclusion: There is no test that indicates the srcset attribute coming from the cms or even a test to add an expectation the sizes attribute on img tag on the high level spec.

I saw here that for the tag you added a helper and the helper spec but I still think that we can test in a high level test (a view spec or even a cucumber spec).

There are some examples on the app for view specs too: https://github.com/moneyadviceservice/frontend/blob/master/spec/views/header_sign_in_box_spec.rb

Or there are examples of cucumber specs that uses VCR to make the request to CMS too on the features dir.

Can we not add a view spec or a cucumber spec for this feature?

Tr4pSt3R · 2017-03-15T09:51:34Z

@tomas-stefano have you looked here and here?

I have put it in [WIP]. Here is a view spec that tests the image srcset size property. The attribute srcset has been tested in this file spec/lib/core/repository/cms/attribute_builder_spec.rb. See here

tomas-stefano · 2017-03-15T11:02:44Z

Since what I saw on the test, the view spec is not testing neither the srcset nor the sizes attribute on img tag. Or there is no integration test that makes the request to the CMS and asserts the the insertion of the srcset attribute (based on the CMS request) and add the sizes attributes as well. I think it is best we talk in person and pair together on the subject.

tomas-stefano · 2017-03-15T11:03:59Z

spec/views/resize_image_spec.rb

+
+		allow(view).to receive(:items_without_image) do
+			[{"heading"=>"Estimate the cost of buying a house and moving", "url"=>"https://example.com/blog", "content"=>"There is more to it than just mortgage repayments"}]
+		end


Can you run rubocop, please? I would fix this identations.

@tomas-stefano Rubocop is now passing for this file Thanks.

* Adds the ability to pull in dynamic srcset properties from the CMS Homepage API. Browsers that support image srcset will serve the right images based on the device that calls the page. * The attribute builder returns the srcset property

Tr4pSt3R · 2017-03-15T16:59:30Z

♻️
@tomas-stefano

I thought about adding a Cucumber spec but it won't achieve anything so I will leave it. The view spec provides sufficient coverage for this.

tomas-stefano · 2017-03-15T17:01:05Z

Thank you for pairing. But now the view spec is testing the core feature and if you remove the tests will fails, exactly what we expected. Saying that, good work mate! 👍 👍 🚢

Tr4pSt3R mentioned this pull request Mar 1, 2017

[DEV APPROVED]7938 resize blog homepage images moneyadviceservice/cms#383

Merged

rnalexander changed the title ~~7938 resize promo article images~~ [WIP]7938 resize promo article images Mar 1, 2017

Tr4pSt3R force-pushed the 7938-resize_promo_article_images branch from e5378d2 to dde43c3 Compare March 3, 2017 08:58

Tr4pSt3R changed the title ~~[WIP]7938 resize promo article images~~ 7938 resize promo article images Mar 3, 2017

norm reviewed Mar 3, 2017

View reviewed changes

Tr4pSt3R force-pushed the 7938-resize_promo_article_images branch from dde43c3 to 2ecdc67 Compare March 3, 2017 10:28

tomas-stefano reviewed Mar 6, 2017

View reviewed changes

tomas-stefano changed the title ~~7938 resize promo article images~~ [WIP] - 7938 resize promo article images Mar 8, 2017

Tr4pSt3R changed the title ~~[WIP] - 7938 resize promo article images~~ 7938 resize promo article images Mar 9, 2017

Tr4pSt3R force-pushed the 7938-resize_promo_article_images branch from 7e66b81 to 068305c Compare March 10, 2017 16:08

Tr4pSt3R changed the title ~~7938 resize promo article images~~ [DEV APPROVED] 7938 resize promo article images Mar 10, 2017

Tr4pSt3R changed the title ~~[DEV APPROVED] 7938 resize promo article images~~ [WIP] 7938 resize promo article images Mar 15, 2017

tomas-stefano reviewed Mar 15, 2017

View reviewed changes

Adds support for serving image srcsets

9d20475

* Adds the ability to pull in dynamic srcset properties from the CMS Homepage API. Browsers that support image srcset will serve the right images based on the device that calls the page. * The attribute builder returns the srcset property

Tr4pSt3R force-pushed the 7938-resize_promo_article_images branch from 65cc25b to 9d20475 Compare March 15, 2017 16:57

Tr4pSt3R changed the title ~~[WIP] 7938 resize promo article images~~ [DEV APPROVED] 7938 resize promo article images Mar 15, 2017

Tr4pSt3R merged commit e4550bd into master Mar 16, 2017

Tr4pSt3R deleted the 7938-resize_promo_article_images branch March 16, 2017 14:14

[DEV APPROVED] 7938 resize promo article images #1691

[DEV APPROVED] 7938 resize promo article images #1691

Conversation

Tr4pSt3R commented Mar 1, 2017 • edited Loading

norm commented Mar 1, 2017

rnalexander commented Mar 1, 2017

Tr4pSt3R commented Mar 3, 2017

norm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tr4pSt3R Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

tomas-stefano Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomas-stefano Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomas-stefano Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmcostello commented Mar 9, 2017

Tr4pSt3R commented Mar 10, 2017

Tr4pSt3R commented Mar 10, 2017

jriga commented Mar 10, 2017

pmcostello commented Mar 10, 2017

Tr4pSt3R commented Mar 10, 2017

jriga commented Mar 10, 2017

tomas-stefano commented Mar 14, 2017 • edited Loading

Tr4pSt3R commented Mar 15, 2017 • edited Loading

tomas-stefano commented Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tr4pSt3R commented Mar 15, 2017

tomas-stefano commented Mar 15, 2017

Tr4pSt3R commented Mar 1, 2017 •

edited

Loading

Tr4pSt3R Mar 3, 2017 •

edited

Loading

tomas-stefano Mar 6, 2017 •

edited

Loading

tomas-stefano Mar 7, 2017 •

edited

Loading

tomas-stefano Mar 6, 2017 •

edited

Loading

tomas-stefano commented Mar 14, 2017 •

edited

Loading

Tr4pSt3R commented Mar 15, 2017 •

edited

Loading

tomas-stefano commented Mar 15, 2017 •

edited

Loading