feat: UI modifications to the article template #856

jcbcapps · 2022-11-08T14:15:44Z

SC-1115

Proposed changes

A user will see the following new components on the article UI:

Label(s)
Byline
Location
Tags

Reviewer notes

To see these changes in the UI:

Create an article (category can be either Internal News or ORBIT Blog)
Add a label, byline, location and tag
On the portal, navigate to your article and verify that each of the items are displayed in the article and match the screenshot below

Setup

As always, run both the portal client and the CMS on your local machine

Code review steps

As the original developer, I have

As code reviewer(s), I have

As a designer reviewer, I have

As a test user, I have

Screenshots

shortcut-integration · 2022-11-08T14:15:47Z

This pull request has been linked to Shortcut Story #1115: UI modifications to the article template.

jcbcapps · 2022-11-08T15:25:32Z

src/components/SingleArticle/SingleArticle.tsx

+    // TODO: Find a way to destructure this.
+    // I think I need to use nullish coalescing here,
+    // but can't figure out how to write it properly
+    // byline: { name: byline },
+    // location: { name: location },


I made byline and location optional in the type, but can't figure out how to destructure them properly. I tried a few things: a ternary, default params, nullish coalescing, but none of that worked. Would love some input on this, but if we can't get it, then it works just fine declaring the old way as you can see below on lines 52, 55, 59 and 62.

destructuring all the way would be nice, but I'm fine if we just keep it to the following and then byline.name / location.name down in the code later. No need to get too complex with the destructuring, imo.

Suggested change

// TODO: Find a way to destructure this.

// I think I need to use nullish coalescing here,

// but can't figure out how to write it properly

// byline: { name: byline },

// location: { name: location },

byline,

location,

Guess the other option is to not make them optional, but that feels unnecessary since they are optional fields on article.

malworks

Hurray we have content identifiers! Two things I noticed:

It doesn't look like the category label is being displayed
The order of the content identifiers should go from global to more granular (Category, Label, Tag) Right now it looks like it is going (Tag, Label)

malworks

🎉

gidjin

Left a comment around the destructuring todo, but other than that looks good to me :)

gidjin · 2022-11-16T21:32:02Z

src/components/SingleArticle/SingleArticle.tsx

-              {dateFormatter.format(publishedDateObj)}
-            </time>
-          </dd>
+          <div>


thought: I was at first confused by the div in the dl but turns out it is valid html and there for grouping. https://www.stefanjudis.com/today-i-learned/divs-are-valid-elements-inside-of-a-definition-list/

Thanks for mentioning this! I meant to add a comment about it, as I was initially having trouble grouping the dl elements until I found the same docs.

gidjin · 2022-11-16T21:45:57Z

src/components/SingleArticle/SingleArticle.tsx

+    // TODO: Find a way to destructure this.
+    // I think I need to use nullish coalescing here,
+    // but can't figure out how to write it properly
+    // byline: { name: byline },
+    // location: { name: location },


destructuring all the way would be nice, but I'm fine if we just keep it to the following and then byline.name / location.name down in the code later. No need to get too complex with the destructuring, imo.

Suggested change

// TODO: Find a way to destructure this.

// I think I need to use nullish coalescing here,

// but can't figure out how to write it properly

// byline: { name: byline },

// location: { name: location },

byline,

location,

gidjin · 2022-11-16T22:01:26Z

src/components/SingleArticle/SingleArticle.tsx

+    // TODO: Find a way to destructure this.
+    // I think I need to use nullish coalescing here,
+    // but can't figure out how to write it properly
+    // byline: { name: byline },
+    // location: { name: location },


Guess the other option is to not make them optional, but that feels unnecessary since they are optional fields on article.

abbyoung · 2022-11-17T17:12:15Z

src/components/SingleArticle/SingleArticle.tsx

@@ -21,16 +27,58 @@ export const SingleArticle = ({ article }: { article: ArticleRecord }) => {

  return (
    <article className={styles.SingleArticle}>
-      <div className={styles.title}>
+      <div>


Q: Where do ORBIT Blog articles fit into this? Is that considered internal news? I know there are two separate categories in the CMS for internal news vs orbit blog, but don't see ORBIT Blog reflected in the constants in this repo. Did we miss something in the past, or am I getting the information architecture confused?

Great question. I think we maybe missed this in the past? I think I added in the constants when creating the internal news page since we were trying to differentiate between what is public-facing and what isn't. But I don't believe the ORBIT Blog was available at that point? Since the blog is just updates that we are making, and the portal is open-source, it doesn't seem like it should be considered "internal." I guess this would ultimately be a design decision as to whether or not we need a new tag for this? Maybe we can discuss in our sync today.

## [4.11.0](4.10.0...4.11.0) (2022-11-30) ### Features * Create USSF Documentation page ([#860](#860)) ([6c5d6b3](6c5d6b3)) * UI modifications to the article template ([#856](#856)) ([dc7402c](dc7402c))

## [4.12.0](4.10.0...4.12.0) (2022-12-13) ### Features * Add support for hero image in articles ([#862](#862)) ([56721db](56721db)) * add USSF Guardian Commitment Poster to docs page ([#888](#888)) ([07fb472](07fb472)) * Create USSF Documentation page ([#860](#860)) ([6c5d6b3](6c5d6b3)) * Display hero image in article layout ([#884](#884)) ([b2201ac](b2201ac)) * UI modifications to the article template ([#856](#856)) ([dc7402c](dc7402c)) ### Bug Fixes * **deps:** update dependency axios to v1 ([#851](#851)) ([de30320](de30320)) ### Security Improvements * **deps:** update node.js to v14.21.1 ([#849](#849)) ([46aba91](46aba91))

jcbcapps added 5 commits November 7, 2022 13:25

Update query to include byline, labels, location and tags

a5f15f5

Update types

43b98c3

Update query for labels and display byline and location

29662dd

Add tags

d7ec82c

Remove labels

0fb82a3

jcbcapps added 2 commits November 8, 2022 09:10

Make byline, location and tags optional

ea22c83

Remove unnecessary <span> from <BreadcrumbNav />

2a1852e

jcbcapps commented Nov 8, 2022

View reviewed changes

jcbcapps and others added 4 commits November 8, 2022 09:26

Merge branch 'main' into sc-1115/updates-to-article-template

c490819

Merge branch 'main' into sc-1115/updates-to-article-template

17b52d6

Add id for tags

1a0be60

Add Label

ac92664

jcbcapps marked this pull request as ready for review November 8, 2022 20:07

jcbcapps requested review from a team November 8, 2022 20:10

malworks suggested changes Nov 9, 2022

View reviewed changes

jcbcapps and others added 2 commits November 10, 2022 09:21

Merge branch 'main' into sc-1115/updates-to-article-template

487c4bf

Add Category and change display order for labels and tags

5e5c147

jcbcapps requested a review from malworks November 15, 2022 20:16

malworks approved these changes Nov 16, 2022

View reviewed changes

gidjin approved these changes Nov 16, 2022

View reviewed changes

Merge branch 'main' into sc-1115/updates-to-article-template

ee5b240

jcbcapps requested review from abbyoung, esacteksab and minhlai as code owners November 17, 2022 15:14

Remove comments and update destructuring

7fc1b2f

abbyoung reviewed Nov 17, 2022

View reviewed changes

jcbcapps added 2 commits November 17, 2022 15:53

Add a default category tag to display

a1d2459

Add category to mock

ecb5b06

Merge branch 'main' into sc-1115/updates-to-article-template

9a67c03

jcbcapps merged commit dc7402c into main Nov 18, 2022

jcbcapps deleted the sc-1115/updates-to-article-template branch November 18, 2022 18:28

gidjin mentioned this pull request Nov 30, 2022

chore(release): 4.11.0 #875

Merged

gidjin mentioned this pull request Dec 13, 2022

chore(release): 4.12.0 #890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: UI modifications to the article template #856

feat: UI modifications to the article template #856

jcbcapps commented Nov 8, 2022 •

edited by malworks

Loading

shortcut-integration bot commented Nov 8, 2022

jcbcapps Nov 8, 2022

gidjin Nov 16, 2022

gidjin Nov 16, 2022

malworks left a comment

malworks left a comment

gidjin left a comment

gidjin Nov 16, 2022

jcbcapps Nov 17, 2022

gidjin Nov 16, 2022

gidjin Nov 16, 2022

abbyoung Nov 17, 2022

jcbcapps Nov 17, 2022

feat: UI modifications to the article template #856

feat: UI modifications to the article template #856

Conversation

jcbcapps commented Nov 8, 2022 • edited by malworks Loading

SC-1115

Proposed changes

Reviewer notes

Setup

Code review steps

As the original developer, I have

As code reviewer(s), I have

As a designer reviewer, I have

As a test user, I have

Screenshots

shortcut-integration bot commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malworks left a comment

Choose a reason for hiding this comment

malworks left a comment

Choose a reason for hiding this comment

gidjin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbcapps commented Nov 8, 2022 •

edited by malworks

Loading