Feature/need help

[jgpawletko]

Overview: Implement functionality for this feature

Highlights

• write Cucumber Feature, Scenarios`, Step definitions, and helper functions for user story cases
• implement partials for Need help? links
• implement layout for Need help? modal dialog

[Finishes #90017830]

[jgpawletko]

@barnabyalter

Hi Barnaby, I'd love feedback on this PR when you get a chance.

I didn't use the Blacklight ajax_modal, rather I wrote up a layout that is reused for both modals. I had trouble getting the Blacklight modal to work and I wanted to deliver the feature, so I proceeded with the layout approach.

Also, since the feature is all view related, I didn't write up any RSpec examples, rather all testing is done in Cucumber.

Thanks for your time!

Best- Joe

[coveralls]

Coverage remained the same at 97.13% when pulling a5981baa55e2f06c8c8308ccdbda00e542f42db3 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[coveralls]

Coverage remained the same at 97.13% when pulling a7bdcd9f3069c370f2225378da8b4956fdb25357 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[coveralls]

Coverage remained the same at 97.13% when pulling a7bdcd9f3069c370f2225378da8b4956fdb25357 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[coveralls]

Coverage remained the same at 97.13% when pulling 8d265aa5e4e50322ee5513e24f7b61dbb1936ec0 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[barnabyalter]

@jgpawletko This is good but I think it would be better if it was unobtrusive JS which would involve creating links to actual pages for search_tips and contact_information and then you can add the rails :remote => true option to link_to and then get some universal coffee to do the rest.

• Change need_help to help and shorten to contact_information and search_tips
• Create a PagesController and some routes, the controller could be empty and just for routing static pages:

class PagesController < ApplicationController
end

get 'help/search_tips', as: :search_tips
get 'help/contact_information', as: :contact_information

Then the _links partial could just link like the following:

<li><%= link_to 'Search Tips', search_tips_path, remote: true %></li>

• Check out what those static pages look like and change the CSS accordingly.
• Delete the custom ajax_modal and just reuse the default blacklight one (for reference, you can find this in blacklight/app/views/shared/_ajax_modal) and then add the following coffeescript to the coffee asset:

$ ->
  $('a[data-remote=true]').click ->
    $('#ajax-modal .modal-content').append($('<div />').addClass('modal-header').append($('<h3 />').addClass('modal-title').html($(this).html())))
    $('#ajax-modal .modal-header').prepend($('<button />').attr({type: 'button', class: 'close'}).data({dismiss: 'modal'}).html('&times;'))
    $('#ajax-modal .modal-content').append($('<div />').addClass('modal-body').load($(this).attr('href')))
    $('#ajax-modal').modal()

This mimics the existing markup that existing stylesheets will look for so it will save some cleanup of css time. It should also DRY things up.

Let me know if this makes sense and is helpful.

[jgpawletko]

@barnabyalter Many thanks for the guidance, Barnaby. I'll move forward with your suggestions.

[jgpawletko]

@barnabyalter Hi Barnaby,

Per your suggestion, I changed the implementation to use the Blacklight ajax_modal. However, I decided to follow the Rails "golden path" and generated a HelpController with search_tips and contact_information actions instead of a PagesController. I can still change the name to PagesController if you'd prefer.

I looked through the Blacklight links you provided earlier and followed the guidance here and here which was easy to implement once I had dedicated URLs for the modal bodies. The links partial is quite straightforward now.

One other thing to note: My initial request specs were failing due to login redirects. I added user login borrowed from the Cucumber setup.

Please let me know if you have any comments or concerns on the revised code.

Thanks! Best- Joe

[coveralls]

Coverage increased (+0.15%) to 97.28% when pulling 56ed230affd202d4b3f5dd3edb1bb782ca683cd5 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[barnabyalter]

@jgpawletko Refactor looks good. You can remove the empty functions in the HelpController since they don't do anything and the route will look for the view regardless of an empty action.

I'm not crazy about embedding the modal classes in a plain html document like you did, it then becomes obtrusive mockup when you visit the non-modal at http://localhost:3000/help/search_tips. I find it cleaner if the html pages can exist on their own. Since the modal is a pure javascript implementation you can abstract it into pure javascript at the global level per my coffeescript example above. It also future proofs new links that we'll want to have be modals.

I notice that my coffeescript example doesn't clear the previous modal content, that can be easily fixed by using html() instead of append():

  $('a[data-remote=true]').click ->
    $('#ajax-modal .modal-content').html($('<div />').addClass('modal-header').append($('<h3 />').addClass('modal-title').html($(this).html())))
    $('#ajax-modal .modal-header').prepend($('<button />').attr({type: 'button', class: 'close'}).data({dismiss: 'modal'}).html('&times;'))
    $('#ajax-modal .modal-content').append($('<div />').addClass('modal-body').load($(this).attr('href')))
    $('#ajax-modal').modal()

It also doesn't need to key off the link title to make the modal header, it can easily key off a header within the html page and then hide that in the modal. This function can be abstracted out and made to accept params if future cases lean that way.

Let me know if you specifically don't want to implement this as coffeescript and we can discuss.

[jgpawletko]

@barnabyalter Thank you for your feedback. I understand your point about obtrusive markup. I'll implement the functionality as coffeescript, but I may have questions for you as I proceed.

[coveralls]

Coverage increased (+0.14%) to 97.27% when pulling 6909e0f6054a6dff1141a74adee03303bb2b3513 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[jgpawletko]

@barnabyalter Hi Barnaby,

Please let me know what you think of this revision.

I had to change one thing in the Coffeescript you provided as the modal wasn't closing. I found that the data-dismiss="modal" attribute was not being inserted into the <button> tag. so I changed this: ... .attr({type: 'button', class: 'close'}).data({dismiss: 'modal'}).html('×')) to this: ... .attr({type: 'button', class: 'close', 'data-dismiss': 'modal'}).html('×'))

Thank you for your time!

[barnabyalter]

@jgpawletko Looks great Joe! And passed all tests. Please make the one change I mentioned to the coffeescript and this is ready to merge.

[jgpawletko]

@barnabyalter Thank you, Barnaby. Requested change made.

[coveralls]

Coverage increased (+0.14%) to 97.27% when pulling b06fd3bf7e3a9d429c2c8a2f9dcdfb8274accf72 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

[coveralls]

Coverage increased (+0.14%) to 97.27% when pulling b06fd3bf7e3a9d429c2c8a2f9dcdfb8274accf72 on feature/need-help into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.