LinkTag: correctly display organization avatar #13928

meg-gutshall · 2021-06-07T07:12:50Z

What type of PR is this? (check all applicable)

Description

This was part of the Live Code Pairing series with @nickytonline and @cmgorton. For this stream we worked on fixing the way avatars appear in liquid tag links when a post is written by a user, but under an organization account. The avatar was not rendering as displayed on the front page, it was only showing the user's avatar.

Related Tickets & Documents

Fixes #11880

QA Instructions, Screenshots, Recordings

This is what we were looking for on the Liquid tags:

This is the issue that we were seeing:

Here is the fix:

Here's an alternate with the author pointing solely to the organization:

UI accessibility concerns?

The images still retain alt tags and they're both wrapped in the hyperlink that will redirect to the article's author.

Added tests?

Yes
No, and this is why: The existing tests would need a decent amount of work to get to the granularity needed to actually test if this feature was working and I don't have that kind of time right now.
I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

I've updated the Developer Docs and/or
Admin Guide, or
Storybook (for Crayons components)
I've updated the README or added inline documentation
I will share this change in a Changelog
or in a forem.dev post
I will share this change internally with the appropriate teams
I'm not sure how best to communicate this change and need help
This change does not need to be communicated, and this is why not: please
replace this line with details on why this change doesn't need to be
shared

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

github-actions · 2021-06-07T07:13:03Z

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

jgaskins

Hey Meg, thanks for the PR! 💯 It looks great! Just a couple nitpicks — and one of them is just a suggestion that I'm not even sure is feasible. 😂

Admittedly I haven't watched the full stream so I don't know if y'all talked about these changes or not. Either way, it was great to see a Ruby for Good friend come through my GitHub notifications. 🙌🏼

jgaskins · 2021-06-07T20:49:16Z

db/schema.rb

    t.datetime "updated_at", precision: 6, null: false
-    t.integer "user_id"
+    t.integer "user_id", null: false


Doesn't look like there are any migrations included here. Can we remove the diff for db/schema.rb?

Sure thing! I think that was automatically added when I ran the migrations. (I couldn't get Docker to cooperate with me so I ended up building locally instead.)

@meg-gutshall I fixed this for you so we can get this PR merged 😃

jgaskins · 2021-06-07T20:54:22Z

app/views/articles/_liquid.html.erb

+  <% if article.organization %>
+    <a href='<%= article.organization.path %>' class='ltag__link__link'>
+      <div class='ltag__link__org__pic'>
+        <img src='<%= Images::Profile.call(article.organization.profile_image_url, length: 150) %>' alt='<%= "#{article.organization.name} image" %>'>
+        <div class='ltag__link__user__pic'>
+          <img src='<%= Images::Profile.call(article.user.profile_image_url, length: 150) %>' alt='<%= "#{article.user.username} image" %>'>
+        </div>
      </div>
-    </div>
-  </a>
+    </a>
+    <a href='<%= article.path %>' class='ltag__link__link'>
+      <div class='ltag__link__content'>
+        <h2><%= title %></h2>
+        <h3><%= article.user.name %> for <%= article.organization.name %> ・ <%= article.readable_publish_date %> ・ <%= article.reading_time < 2 ? 1 : article.reading_time %> min read</h3>
+        <div class='ltag__link__taglist'>
+        <% article.tag_list.each do |t| %>
+          <span class='ltag__link__tag'>#<%= t %></span>
+        <% end %>
+        </div>
+      </div>
+    </a>


Would it make sense to put something like this above and share more of the markup between the if and else blocks?

<% owner = article.organization || article.user %> [same as existing markup but change `article.user` -> `owner`]

And then we could scope the if block down to just the part where it shows the org+user vs user avatars. Maybe it's that I don't do enough front-end work anymore, but I'm not super great at seeing what's different between two 20-line chunks of HTML. 🙂

Note: I tried to add a suggested-change block here to make the change a 2-click operation for you but apparently GitHub doesn't support that when lines have been deleted in the existing diff. 😕

Okay, I'll give this another go and see what I can do. I was fading when I got this in last night so I can probably refactor a bit in here.

meg-gutshall · 2021-06-07T22:21:49Z

Oh hi there @jgaskins! Yes, so lovely to see another gooder! 😁

BTW, when I tried to run the RSpec tests last night before submitting, I saw that the test db hadn't been created in the initial setup. Is there a different command I need to run for that?

Thank you for the helpful feedback! I will try to make time this weekend to submit an update.

citizen428 · 2021-06-08T03:05:54Z

I saw that the test db hadn't been created in the initial setup. Is there a different command I need to run for that?

Hmm, did you use our bin/setup script? Then it should have been created. You can also try to run rails db:test:prepare.

aitchiss

I tested this locally and it's working great!

I made sure there's no regression for links in posts published before the changes, as well as for newly added ones, and all looks good to me 👍

I just have a couple of very minor change requests to enhance the experience for screen reader users. Let me know if you have any questions, and thanks very much for these changes!

app/views/articles/_liquid.html.erb

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

meg-gutshall · 2021-06-14T03:24:30Z

@aitchiss I appreciate the work you put into the testing portion of this! I agree that alt tag content sounds much better. I've accepted both of your suggested changes.

@citizen428 I did run bin/setup per the instructions but somehow the test database did not get created, only the development database. Here is the content of my test.log file if that helps any:

Unset ENV variable: IMGPROXY_KEY.
Unset ENV variable: IMGPROXY_SALT.
Unset ENV variable: RELEASE_FOOTPRINT.
** [Honeybadger] Initializing Honeybadger Error Tracker for Ruby. Ship it! version=4.8.0 framework=rails level=1 pid=34131
Unset ENV variable: COMMUNITY_COPYRIGHT_START_YEAR.

aitchiss · 2021-06-14T12:45:02Z

@meg-gutshall - this is looking great - thanks for making those tweaks!

I noticed there are some specs failing

The "frontend tests" build job failed due to a known flakey test - I don't think it's anything to do with your code and I've restarted it now 🙂
The other failed build job is because of the changes to the alt tag - in spec/liquid_tags/link_tag_spec.rb we'll need to update the correct_link_html to reflect the new alt text. Let me know if you have any trouble doing this - hopefully just a quick change!

Thanks again - once the specs are passing I think this is good to go 🚀

citizen428 · 2021-06-21T03:16:12Z

Hi @meg-gutshall thanks for all your work on this PR so far, it's much appreciated! 🥇 Is there anything we can do to help you push this over the finish line?

meg-gutshall · 2021-06-21T16:50:20Z

Hi @aitchiss and @citizen428! Thank you for your help and patience. I'm finishing up a coding bootcamp with a deadline of 6/30 (and just starting my final project today 😬 ) so I need to prioritize that.

I can finish up this PR in early July if that's okay with you. If it can't wait, I'm okay with co-authoring this PR. I feel like @aitchiss should get co-author recognition if she hasn't already for her suggested changes.

Please let me know how you would like to proceed!

citizen428 · 2021-06-22T01:50:05Z

I'm fine either way, I'll leave it up to @aitchiss 😃

aitchiss · 2021-06-22T09:40:56Z

That's exciting about your final project @meg-gutshall!

We're not in any rush, but I think there's only a teeny change to the specs needed, so I'm happy to have a look at that and then hopefully we can get this merged 😄 You've already done 99.9% - happy to help push it over the line!

…ag-company-avatars-11880

aitchiss

I pushed up that small change to fix the specs, and all seems to be passing now 🎉

Nice work on this one! ✨

meg-gutshall · 2021-06-22T15:09:21Z

Woohoo! Thank you tons! Time for a celebration dance!!!

cmgorton · 2021-06-22T15:55:48Z

Woohoo! Thank you tons! Time for a celebration dance!!!

Woohoo! Thanks for the contribution Meg🎉

And thanks team for all the help 🙏

nickytonline · 2021-06-22T17:20:04Z

Nice to see this completed @meg-gutshall! I'm going to link the stream to this PR for folks to see it come full circle.

Thanks again for your contribution to the Forem codebase! 😎

meg-gutshall added 5 commits May 8, 2021 17:33

Set up Docker

69d81ab

Merge remote-tracking branch 'upstream/main'

f25aa0d

Merge remote-tracking branch 'upstream/main'

567314a

Add conditional to render org logo with user avatar

0978a33

Create styling for logo with avatar

98a20ac

meg-gutshall requested review from a team June 7, 2021 07:12

meg-gutshall requested a review from a team as a code owner June 7, 2021 07:12

meg-gutshall requested review from joshpuetz, citizen428, Ridhwana and aitchiss and removed request for a team June 7, 2021 07:12

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jun 7, 2021

jgaskins reviewed Jun 7, 2021

View reviewed changes

rhymes changed the title ~~Meg gutshall/liquid tag company avatars 11880~~ LinkTag: correctly display organization avatar Jun 9, 2021

aitchiss suggested changes Jun 11, 2021

View reviewed changes

app/views/articles/_liquid.html.erb Outdated Show resolved Hide resolved

app/views/articles/_liquid.html.erb Outdated Show resolved Hide resolved

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Jun 11, 2021

Update app/views/articles/_liquid.html.erb

5b04043

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Jun 14, 2021

Update app/views/articles/_liquid.html.erb

e677a14

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

rhymes requested review from aitchiss and jgaskins June 14, 2021 07:30

aitchiss added 2 commits June 22, 2021 11:13

update rspec test

113c20c

Merge remote-tracking branch 'origin/main' into meg-gutshall/liquid-t…

10a7414

…ag-company-avatars-11880

aitchiss approved these changes Jun 22, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jun 22, 2021

Restore schema.rb

d433c95

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jun 23, 2021

citizen428 approved these changes Jun 23, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jun 23, 2021

citizen428 merged commit ef474fd into forem:main Jun 23, 2021

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LinkTag: correctly display organization avatar #13928

LinkTag: correctly display organization avatar #13928

meg-gutshall commented Jun 7, 2021

github-actions bot commented Jun 7, 2021

jgaskins left a comment

jgaskins Jun 7, 2021

meg-gutshall Jun 7, 2021

citizen428 Jun 23, 2021

jgaskins Jun 7, 2021

meg-gutshall Jun 7, 2021

meg-gutshall commented Jun 7, 2021

citizen428 commented Jun 8, 2021

aitchiss left a comment

meg-gutshall commented Jun 14, 2021

aitchiss commented Jun 14, 2021

citizen428 commented Jun 21, 2021

meg-gutshall commented Jun 21, 2021

citizen428 commented Jun 22, 2021

aitchiss commented Jun 22, 2021

aitchiss left a comment

meg-gutshall commented Jun 22, 2021

cmgorton commented Jun 22, 2021

nickytonline commented Jun 22, 2021

LinkTag: correctly display organization avatar #13928

LinkTag: correctly display organization avatar #13928

Conversation

meg-gutshall commented Jun 7, 2021

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added tests?

[Forem core team only] How will this change be communicated?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

github-actions bot commented Jun 7, 2021

jgaskins left a comment

Choose a reason for hiding this comment

jgaskins Jun 7, 2021

Choose a reason for hiding this comment

meg-gutshall Jun 7, 2021

Choose a reason for hiding this comment

citizen428 Jun 23, 2021

Choose a reason for hiding this comment

jgaskins Jun 7, 2021

Choose a reason for hiding this comment

meg-gutshall Jun 7, 2021

Choose a reason for hiding this comment

meg-gutshall commented Jun 7, 2021

citizen428 commented Jun 8, 2021

aitchiss left a comment

Choose a reason for hiding this comment

meg-gutshall commented Jun 14, 2021

aitchiss commented Jun 14, 2021

citizen428 commented Jun 21, 2021

meg-gutshall commented Jun 21, 2021

citizen428 commented Jun 22, 2021

aitchiss commented Jun 22, 2021

aitchiss left a comment

Choose a reason for hiding this comment

meg-gutshall commented Jun 22, 2021

cmgorton commented Jun 22, 2021

nickytonline commented Jun 22, 2021