Fixes #2001: Added bottom padding to improve visual break #2130

rjayroso · 2021-04-12T05:20:50Z

Issue This PR Addresses

Fixes #2001

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

I've thought about how to improve the visual break between posts. I've looked at how Instagram separates their infinite timeline of posts and it turns out that they don't (unlike tumblr). I think that the easiest solution would just to simply give it more space, see below:

d5194b2a9b48e9400f05af91ad8f75c2.mp4

From my reasoning, the next post's title looks like a subtitle for the current post is because the spacing is basically equal and the user struggles to identify who the title belongs to. By adding more space between, it'll be easier to see the title belongs to the post below rather than the previous post above.

Let me know if we want a more drastic design change to improve the visual break or if the gap should be shortened or made larger.

How to Test

View the file change, ask the following:
- should I change the unit of measurement? (px instead of rem?)
View deployment, ask the following:
- is the extra space enough? too little?
- should this warrant a more drastic design change?

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

PedroFonsecaDEV · 2021-04-12T09:38:11Z

@rjayroso @humphd

I think that increasing the space between the posts is not enough. We need to add something else.
Follow my suggestions:

1 2 (I'm using the wrong colors here).

gif:

img:

PedroFonsecaDEV

Please check the comment above.

rjayroso · 2021-04-16T07:11:30Z

@PedroFonsecaDEV I may have encountered a bug that was giving me trouble, though it may be intentional. That aside, do we still want to put the avatar at the bottom? I think when it is scaled down it will be hard to see and just be noise. I added the line and the "Written by: author" though and copied the colours and border width from the author container on the side, let me know what you think.

84f0283799b5cb18030dff197908838d.mp4

As for the aforementioned bug, there is an invisible image at the end of all medium posts.

.telescope-post-content img[src*='medium.com/_/stat'] {
  width: 1px;
  height: 1px;
}

If we put a line at the bottom, the gap will appear larger (specifically to medium posts).
I am unsure if I should remove it since I don't know if it serves a purpose elsewhere, thoughts?

PedroFonsecaDEV

I made some comments.
Regarding the medium ghost image, we can deal with it in a follow-up. Let's keep the scope of this PR.

src/web/src/components/Posts/Post.tsx

PedroFonsecaDEV · 2021-04-16T09:38:33Z

src/web/src/components/Posts/Post.tsx

@@ -302,6 +308,14 @@ const PostComponent = ({ postUrl }: Props) => {
          dangerouslySetInnerHTML={{ __html: post.html }}
        />
      </div>
+      <div className={classes.contentFooter}>
+        <p>
+          Written by:{' '}


Let's add the <TelescopeAvatar> as well.

chrispinkney · 2021-04-16T15:27:43Z

src/web/src/components/Posts/Post.tsx

@@ -302,6 +308,14 @@ const PostComponent = ({ postUrl }: Props) => {
          dangerouslySetInnerHTML={{ __html: post.html }}
        />
      </div>
+      <div className={classes.contentFooter}>
+        <p>
+          Written by:{' '}


I'm not sure how I feel about Written by, we already show the user on the right, why add it again at the bottom? I do think the line is nice though.

What do others think?

humphd · 2021-04-16T17:55:28Z

What if we try only doing the bar with no extra info about the user or post for now, and iterate on that? That seems to be the general feedback I'm hearing on our call today.

huyxgit · 2021-04-16T21:02:30Z

@rjayroso @humphd I'll stick with the idea I mentioned earlier in the call:

Keep the header and add a line like the picture below (+ some margin to the upper post), OR don't need the line anymore because the title+authorInfo will play a role like a line better this case, this not only help 2 separate posts more visible but also we can remove the current author info on right side. That place, instead of author info (which is a waste of space when it's not even visible on mobile/small screen), we can use that space (right) for the share feature in future, or a 'monthly/yearly timeline' (help user browsing old posts by that specific month/year when click on, sth like 'archive' on wordpress sites).

rjayroso · 2021-04-16T23:45:49Z

@PedroFonsecaDEV @huynguyez @chrispinkney, what do you think of the latest deployment? I think for the whole removing the sidebar thing should be done in another issue, lets just get the line and spacing for the visual break.

Anyways, should the line be close to the top post, in the middle, or as Huy suggested - closer to the bottom? Also: any comments on making a larger/smaller gap, darker/lighter, thinner/bolder line etc?

chrispinkney

Looks great! Just needs some more margin-top I think, since the gap between the next post and the end of the current post are noticeably different. If others are fine with it then I don't mind giving an approval!

I'm not sure what a darker/bolder line would look like but I'm curious to try it. Feel free to suggest some css changes I can apply to the deployment via the browser!

* Added bottom padding * Added bottom border and author in the footer of each post * Removed author, kept the line * Added more margin at the top of the line

rjayroso · 2021-04-19T03:33:34Z

Looks great! Just needs some more margin-top I think, since the gap between the next post and the end of the current post are noticeably different. If others are fine with it then I don't mind giving an approval!

I'm not sure what a darker/bolder line would look like but I'm curious to try it. Feel free to suggest some css changes I can apply to the deployment via the browser!

@chrispinkney done n done! rebased and squashed too

PedroFonsecaDEV

LGTM.

rjayroso added type: enhancement New feature or request area: front-end area: design Issues needing design or assets labels Apr 12, 2021

rjayroso requested review from humphd, huyxgit, yuanLeeMidori and PedroFonsecaDEV April 12, 2021 05:20

rjayroso self-assigned this Apr 12, 2021

vercel bot deployed to Preview April 12, 2021 05:20 View deployment

PedroFonsecaDEV added this to the 2.0 Release milestone Apr 12, 2021

PedroFonsecaDEV requested changes Apr 12, 2021

View reviewed changes

PedroFonsecaDEV added the Priority: High label Apr 16, 2021

vercel bot deployed to Preview April 16, 2021 06:56 View deployment

rjayroso requested a review from PedroFonsecaDEV April 16, 2021 07:11

PedroFonsecaDEV requested changes Apr 16, 2021

View reviewed changes

chrispinkney reviewed Apr 16, 2021

View reviewed changes

vercel bot deployed to Preview April 16, 2021 23:19 View deployment

rjayroso requested review from chrispinkney and PedroFonsecaDEV April 16, 2021 23:46

chrispinkney requested changes Apr 17, 2021

View reviewed changes

vercel bot deployed to Preview April 19, 2021 03:27 View deployment

rjayroso force-pushed the issue-2001 branch from c268850 to 7c82f53 Compare April 19, 2021 03:29

vercel bot deployed to Preview April 19, 2021 03:29 View deployment

Improved visual break between posts

4da1170

* Added bottom padding * Added bottom border and author in the footer of each post * Removed author, kept the line * Added more margin at the top of the line

rjayroso force-pushed the issue-2001 branch from 7c82f53 to 4da1170 Compare April 19, 2021 03:31

vercel bot deployed to Preview April 19, 2021 03:31 View deployment

yuanLeeMidori approved these changes Apr 19, 2021

View reviewed changes

PedroFonsecaDEV approved these changes Apr 19, 2021

View reviewed changes

PedroFonsecaDEV requested a review from chrispinkney April 19, 2021 10:54

rjayroso merged commit 239235f into Seneca-CDOT:master Apr 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2001: Added bottom padding to improve visual break #2130

Fixes #2001: Added bottom padding to improve visual break #2130

rjayroso commented Apr 12, 2021

PedroFonsecaDEV commented Apr 12, 2021

PedroFonsecaDEV left a comment

rjayroso commented Apr 16, 2021

PedroFonsecaDEV left a comment

PedroFonsecaDEV Apr 16, 2021 •

edited

Loading

chrispinkney Apr 16, 2021

humphd commented Apr 16, 2021

huyxgit commented Apr 16, 2021 •

edited

Loading

rjayroso commented Apr 16, 2021

chrispinkney left a comment •

edited

Loading

rjayroso commented Apr 19, 2021

PedroFonsecaDEV left a comment

Fixes #2001: Added bottom padding to improve visual break #2130

Fixes #2001: Added bottom padding to improve visual break #2130

Conversation

rjayroso commented Apr 12, 2021

Issue This PR Addresses

Type of Change

Description

How to Test

Checklist

PedroFonsecaDEV commented Apr 12, 2021

1

2 (I'm using the wrong colors here).

PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

rjayroso commented Apr 16, 2021

PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

PedroFonsecaDEV Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

chrispinkney Apr 16, 2021

Choose a reason for hiding this comment

humphd commented Apr 16, 2021

huyxgit commented Apr 16, 2021 • edited Loading

rjayroso commented Apr 16, 2021

chrispinkney left a comment • edited Loading

Choose a reason for hiding this comment

rjayroso commented Apr 19, 2021

PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

PedroFonsecaDEV Apr 16, 2021 •

edited

Loading

huyxgit commented Apr 16, 2021 •

edited

Loading

chrispinkney left a comment •

edited

Loading