Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LinkTag: correctly display organization avatar #13928

Merged
merged 10 commits into from Jun 23, 2021
Merged

LinkTag: correctly display organization avatar #13928

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
69d81ab
Set up Docker
meg-gutshall May 8, 2021
f25aa0d
Merge remote-tracking branch 'upstream/main'
meg-gutshall May 24, 2021
567314a
Merge remote-tracking branch 'upstream/main'
meg-gutshall Jun 7, 2021
0978a33
Add conditional to render org logo with user avatar
meg-gutshall Jun 7, 2021
98a20ac
Create styling for logo with avatar
meg-gutshall Jun 7, 2021
5b04043
Update app/views/articles/_liquid.html.erb
meg-gutshall Jun 14, 2021
e677a14
Update app/views/articles/_liquid.html.erb
meg-gutshall Jun 14, 2021
113c20c
update rspec test
aitchiss Jun 22, 2021
10a7414
Merge remote-tracking branch 'origin/main' into meg-gutshall/liquid-t…
aitchiss Jun 22, 2021
d433c95
Restore schema.rb
citizen428 Jun 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 53 additions & 0 deletions app/assets/stylesheets/ltags/LinkTag.scss
View file
Open in desktop
@@ -1,4 +1,5 @@
@import '../variables';

.ltag__link__link {
color: $black !important;
color: var(--body-color) !important;
Expand Down Expand Up @@ -54,11 +55,63 @@
display: inline-block;
padding: calc(0.4vw + 8px) calc(0.8vw + 8px);
padding-right: 8px;
box-sizing: border-box;
img {
width: calc(2.2vw + 45px);
height: calc(2.2vw + 45px);
margin: auto auto !important;
border-radius: 150px;
box-sizing: border-box;
}
}
.ltag__link__org__pic {
border-radius: var(--radius);
position: relative;
background-color: var(--body-color-inverted);
display: inline-block;
padding: calc(0.4vw + 8px) calc(0.8vw + 8px);
padding-top: calc(0.5vw + 12px);
padding-right: 8px;
margin-right: 2px;
box-sizing: border-box;
&::after {
content: '';
opacity: 0.15;
width: 100%;
height: 100%;
position: absolute;
left: 0;
top: 0;
border-radius: var(--radius);
pointer-events: none;
box-sizing: border-box;
}
}
.ltag__link__org__pic > img {
display: inline-block;
width: calc(2.5vw + 40px);
height: calc(2.5vw + 40px);
margin: auto auto !important;
border-radius: var(--radius);
box-sizing: border-box;
}
.ltag__link__user__pic {
display: inline-block;
position: absolute;
right: calc(0.5vw * -1);
bottom: calc(1vw);
border: 2px solid var(--base-inverted);
border-radius: 100%;
background-color: var(--card-color-tertiary);
box-sizing: border-box;
width: calc(2vw + 25px);
height: calc(2vw + 25px);
img {
display: inline-block;
width: 100%;
height: 100%;
border-radius: 100%;
margin: 0;
}
}
.ltag__link__content {
Expand Down
52 changes: 37 additions & 15 deletions app/views/articles/_liquid.html.erb
View file
Open in desktop
Expand Up @@ -7,22 +7,44 @@
<%= article.video_duration_in_minutes %></span>
</a>
<% end %>
<a href='<%= article.user.path %>' class='ltag__link__link'>
<div class='ltag__link__pic'>
<img src='<%= Images::Profile.call(article.user.profile_image_url, length: 150) %>' alt='<%= "#{article.user.username} image" %>'>
</div>
</a>
<a href='<%= article.path %>' class='ltag__link__link'>
<div class='ltag__link__content'>
<h2><%= title %></h2>
<h3><%= article.user.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 %>
<% 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>
meg-gutshall marked this conversation as resolved.
Show resolved Hide resolved
</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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<% else %>
<a href='<%= article.user.path %>' class='ltag__link__link'>
<div class='ltag__link__pic'>
<img src='<%= Images::Profile.call(article.user.profile_image_url, length: 150) %>' alt='<%= "#{article.user.username} image" %>'>
meg-gutshall marked this conversation as resolved.
Show resolved Hide resolved
</div>
</a>
<a href='<%= article.path %>' class='ltag__link__link'>
<div class='ltag__link__content'>
<h2><%= title %></h2>
<h3><%= article.user.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>
<% end %>
</div>
<% else %>
<div class='ltag__link'>
Expand Down
6 changes: 3 additions & 3 deletions db/schema.rb
View file
Open in desktop
Expand Up @@ -1341,7 +1341,7 @@
t.datetime "last_article_at", default: "2017-01-01 05:00:00"
t.datetime "last_comment_at", default: "2017-01-01 05:00:00"
t.datetime "last_followed_at"
t.datetime "last_moderation_notification", default: "2017-01-01 05:00:00"
t.datetime "last_moderation_notification", default: "2017-01-01 00:00:00"
t.datetime "last_notification_activity"
t.string "last_onboarding_page"
t.datetime "last_reacted_at"
Expand Down Expand Up @@ -1423,9 +1423,9 @@

create_table "users_gdpr_delete_requests", force: :cascade do |t|
t.datetime "created_at", precision: 6, null: false
t.string "email"
t.string "email", null: false
t.datetime "updated_at", precision: 6, null: false
t.integer "user_id"
t.integer "user_id", null: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

t.string "username"
end

Expand Down