Add story pinned option #975
Conversation
|
This is great to see! Thanks so much. A couple of things:
You can see this in the blue underlines in your GIFs above, as well as the the New button at the top right which is supposed to be orange background, and a bunch of other little things. And on React, the light blue link color at the bottom. Can we find a better way to access the icons without importing a bunch of CSS we either don't need or is impacting the app style?
|
… svg for story pinned
Hello, the problem is that the exact "icon-pushpin" is only presented in version 3 of font-awesome: https://fontawesome.com/v3/icons/ |
|
@griseduardo You think you can rebase your branch so we dont have the conflicts? |
There was a problem hiding this comment.
Overall, this a great start! I do have a few concerns that I've outlined below.
I would also love to see tests added surrounding the actions for pinning and unpinning. While unlikely, it isn't impossible for pinned stories to get out of sync and result in a community having multiple stories pinned. With the way the code is right now, if that were to happen, we would only ever unpin the first found pinned story rather than all of them.
Thank you for your contribution and I apologize for the rather tardy review.
| if params[:story_pinned].present? | ||
| if params[:story_pinned] | ||
| @pinned_story = @stories.find_by(story_pinned: true) | ||
| @pinned_story&.update(story_pinned: !params[:story_pinned]) | ||
| end | ||
|
|
||
| @story.update(story_pinned: params[:story_pinned]) | ||
|
|
||
| return redirect_to action: "index" | ||
| end |
There was a problem hiding this comment.
For ease of maintaining and reducing complexity of this, I think we should split out this block to allow pinning/unpinning from non-form views as its own custom rails action. I might also suggest we create two, one for pinning and one for unpinning and trigger them using remote: true calls so we don't refresh the page on them. Another benefit of this will ensure the logic stays relatively simple:
def pin
story = authorize community_stories.find(params[:id])
community_stories.update_all(story_pinned: false)
story.update(story_pinned: true)
end
def unpin
story = authorize community_stories.find(params[:id])
story.update(story_pinned: false)
endThere was a problem hiding this comment.
Good idea separate in different actions.
Only one problem, related to use remote true, is that it doesn't change the icon color and reordenize the stories, that can pass the wrong idea that the pin or unpin action didn't worked.
I maintained the render behavior. What do you think about it?
| <% if story.story_pinned %> | ||
| <div class="edit-icon-pinned"> | ||
| <%= form_with model: @story, url: story_path(story.id), method: "patch", multipart: true, class: "form", data: {remote: false}, locale: true do |f| %> | ||
| <%= f.hidden_field :story_pinned, value: false %> | ||
| <%= f.button nil, class: "icon-pinned" do %> | ||
| <%= image_tag 'thumbtack-pinned.svg', alt: 'pinned-story', class: "image-pinned" %> | ||
| <% end %> | ||
| <% end %> | ||
| </div> | ||
| <% else %> | ||
| <div class="edit-icon-not-pinned"> | ||
| <%= form_with model: @story, url: story_path(story.id), method: "patch", multipart: true, class: "form", data: {remote: false}, locale: true do |f| %> | ||
| <%= f.hidden_field :story_pinned, value: true %> | ||
| <%= f.button nil, class: "icon-not-pinned" do %> | ||
| <%= image_tag 'thumbtack-not-pinned.svg', alt: 'pinned-story', class: "image-pinned" %> | ||
| <% end %> | ||
| <% end %> | ||
| </div> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
One of the benefits of using an SVG over a standard image is the ability to store them as code rather than image assets. In our views, we have a shared/_icons.html.erb partial that defines shareable SVGs for use in this manner:
<svg hidden xmlns="http://www.w3.org/2000/svg">
<symbol id="icon-thumbtack" attributes="from svg">
<path data />
</symbol>
</svg>
and then referenced like:
<svg><use href="#icon-name"></svg>
These can then be styled with classes as usual, with allowance for using "fill" and "stroke" to color the paths and strokes in.
With a few extra changes, these can also be loaded into the react code and referenced the same way.
There was a problem hiding this comment.
Nice idea, I updated to use the svg this way in rails. Anyway I tried and doesn't find for now a way to use the same in React. I used the svg directly there. Do you have a suggestion for how to make it work?
…nned and icon unpinned
There was a problem hiding this comment.
Thanks for working through all of the feedback! This is looking great. A couple of additional comments in line.
I would also love to see some specs around the pinning and unpinning actions — both the individual pin/unpin endpoints as well as in the create and edit form. Are you able to add those for us?
I will add the tests :) |
|
Hello @rudokemper and @lauramosher |
Motivation
Allow pin story and show first
Changes
Screenshots
Stories dashboard (admin/editor):
Stories dashboard (member):
Story dashboard (admin/editor):
Home page:
