Fixes #2169: Rename blog prop to url in PostAvatar, deal with optional value #2176

rjayroso · 2021-04-19T16:21:03Z

Issue This PR Addresses

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

Changes the PostAvatar blog prop to be named url.
Changes how the PostAvatar is rendered: if there is no url, then PostAvatar is rendered without an <a> element wrapped around it.

How to Test

Pull this branch for testing, go to components/Posts/Post.tsx in VSCode (or your editor of choice) and scroll to the bottom - put a <PostAvatar name="FirstName LastName" /> element somewhere that it can be rendered and seen.

You should see an Avatar with the initials "FL" and it should also be unclickable (since their is no url prop)

Now add the prop url="Test" to the PostAvatar (e.g. <PostAvatar name="FirstName LastName" url="Test"/>

You should see that same Avatar now be clickable (if clicked it will give you Telescope's 404 because it is an invalid url).

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)

HyperTHD · 2021-04-19T16:32:00Z

src/web/src/components/Posts/PostAvatar.tsx

-        </Avatar>
-      </a>
+      <div>
+        {url.length > 0 ? (


I'm not sure about this, but shouldn't we also be checking if url is not null as well, basically if it exists as well as if the length is greater than 0

So, I think from what I understand after reading the PostAvatar.tsx component is that, if no url is given in the props, then the url is assigned to a "" empty string. Either way, I can add it quickly it's no problem

Edit: check the latest commit

Edit2: I just realized there are a lot of ways to check if url is undefined, if you have a better conditional let me know and I can quickly update it

HyperTHD · 2021-04-19T16:41:50Z

src/web/src/components/Posts/PostAvatar.tsx

@@ -59,7 +59,7 @@ const PostAvatar = ({ name, img, url = '' }: AvatarProps) => {
      .join('');
    return (
      <div>
-        {url.length > 0 ? (
+        {url.length > 0 && url != undefined ? (


Think just url.length > 0 && url is fine enough

url?.length && (...)

humphd · 2021-04-19T21:33:10Z

Please update the title of this PR to give info about what it is, in addition to what it fixes.

humphd · 2021-04-19T21:33:51Z

src/web/src/components/Posts/PostAvatar.tsx

@@ -39,7 +39,7 @@ const useStyles = makeStyles((theme: Theme) =>
  })
 );

-const PostAvatar = ({ name, img, blog = '' }: AvatarProps) => {
+const PostAvatar = ({ name, img, url = '' }: AvatarProps) => {


Don't use = '' as a default. If it's not there, we need to know that.

humphd · 2021-04-19T21:34:38Z

src/web/src/components/Posts/PostAvatar.tsx

@@ -59,7 +59,7 @@ const PostAvatar = ({ name, img, url = '' }: AvatarProps) => {
      .join('');
    return (
      <div>
-        {url.length > 0 ? (
+        {url.length > 0 && url != undefined ? (


url?.length && (...)

HyperTHD · 2021-04-20T01:40:42Z

src/web/src/components/Posts/PostAvatar.tsx

-        </Avatar>
-      </a>
+      <div>
+        {url != undefined && url?.length > 0 ? (


This is really gonna sound nit-picky, but should be !== instead of !=

Doing url? is the same as checking if url exists, so you don't need this. However, @HyperTHD is right: you never want to use != or == in JS, since they both allow implicit type casting to happen, whereas !== and === will also check the types.

humphd · 2021-04-20T02:03:13Z

src/web/src/components/Posts/PostAvatar.tsx

-        </Avatar>
-      </a>
+      <div>
+        {url != undefined && url?.length > 0 ? (


Doing url? is the same as checking if url exists, so you don't need this. However, @HyperTHD is right: you never want to use != or == in JS, since they both allow implicit type casting to happen, whereas !== and === will also check the types.

yuanLeeMidori

Just do squash and merge, otherwise looks good for me :)

rjayroso · 2021-04-20T18:02:11Z

Squashed and rebased

deal with optional value * Changed blog prop to url * Changed prop name to url in other files using PostAvatar * Changed PostAvatar to no longer wrapped by an <a> by default * Searched for instances of PostAvatar and changed blog prop to url * Added extra conditional to check if url is not undefined * Removed url defaulting to empty string. Changed the conditional * 'url != undefined' changed to 'url !=== undefined' * Changed 'url !== undefined && url?.length > 0' to 'url?.length'

humphd · 2021-04-20T21:40:45Z

@rjayroso you can rebase and merge this from GitHub, you don't have to keep rebasing manually.

humphd · 2021-04-21T01:15:11Z

@rjayroso wait to merge this til we fix CI, which is failing atm

rjayroso · 2021-04-21T01:24:46Z

@rjayroso wait to merge this til we fix CI, which is failing atm

Yeah no worries, now that I know I can rebase and merge from GitHub I'm ready whenever 😅

rjayroso added type: enhancement New feature or request area: front-end labels Apr 19, 2021

rjayroso requested review from humphd, HyperTHD, yuanLeeMidori and PedroFonsecaDEV April 19, 2021 16:21

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

HyperTHD assigned rjayroso Apr 19, 2021

HyperTHD added this to the 2.0 Release milestone Apr 19, 2021

HyperTHD reviewed Apr 19, 2021

View reviewed changes

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

HyperTHD reviewed Apr 19, 2021

View reviewed changes

humphd requested changes Apr 19, 2021

View reviewed changes

manekenpix changed the title ~~Fixes #2169:~~ Fixes #2169: Rename blog prop to url in PostAvatar, deal with optional value Apr 19, 2021

vercel bot deployed to Preview April 20, 2021 00:13 View deployment

rjayroso requested review from humphd and HyperTHD April 20, 2021 00:21

HyperTHD requested changes Apr 20, 2021

View reviewed changes

humphd requested changes Apr 20, 2021

View reviewed changes

chrispinkney requested a review from izhuravlev April 20, 2021 14:10

vercel bot deployed to Preview April 20, 2021 14:11 View deployment

yuanLeeMidori previously approved these changes Apr 20, 2021

View reviewed changes

rjayroso dismissed yuanLeeMidori’s stale review via a7ba630 April 20, 2021 14:16

rjayroso force-pushed the issue-2169 branch from f9807ce to a7ba630 Compare April 20, 2021 14:16

vercel bot deployed to Preview April 20, 2021 14:16 View deployment

rjayroso requested review from humphd, HyperTHD and yuanLeeMidori April 20, 2021 14:25

humphd previously approved these changes Apr 20, 2021

View reviewed changes

yuanLeeMidori previously approved these changes Apr 20, 2021

View reviewed changes

rjayroso dismissed stale reviews from yuanLeeMidori and humphd via 9d0b5fc April 20, 2021 21:16

rjayroso force-pushed the issue-2169 branch from a7ba630 to 9d0b5fc Compare April 20, 2021 21:16

vercel bot deployed to Preview April 20, 2021 21:16 View deployment

humphd approved these changes Apr 20, 2021

View reviewed changes

rjayroso requested a review from yuanLeeMidori April 20, 2021 21:41

yuanLeeMidori approved these changes Apr 21, 2021

View reviewed changes

PedroFonsecaDEV approved these changes Apr 21, 2021

View reviewed changes

humphd merged commit bc0f2b7 into Seneca-CDOT:master Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2169: Rename blog prop to url in PostAvatar, deal with optional value #2176

Fixes #2169: Rename blog prop to url in PostAvatar, deal with optional value #2176

rjayroso commented Apr 19, 2021

HyperTHD Apr 19, 2021

rjayroso Apr 19, 2021 •

edited

Loading

HyperTHD Apr 19, 2021

humphd Apr 19, 2021

humphd commented Apr 19, 2021

humphd Apr 19, 2021

humphd Apr 19, 2021

HyperTHD Apr 20, 2021

humphd Apr 20, 2021

humphd Apr 20, 2021

yuanLeeMidori left a comment

rjayroso commented Apr 20, 2021

humphd commented Apr 20, 2021

humphd commented Apr 21, 2021

rjayroso commented Apr 21, 2021

Fixes #2169: Rename blog prop to url in PostAvatar, deal with optional value #2176

Fixes #2169: Rename blog prop to url in PostAvatar, deal with optional value #2176

Conversation

rjayroso commented Apr 19, 2021

Issue This PR Addresses

Type of Change

Description

How to Test

Checklist

Choose a reason for hiding this comment

rjayroso Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanLeeMidori left a comment

Choose a reason for hiding this comment

rjayroso commented Apr 20, 2021

humphd commented Apr 20, 2021

humphd commented Apr 21, 2021

rjayroso commented Apr 21, 2021

rjayroso Apr 19, 2021 •

edited

Loading