feat: Add internal news carousel for news and announcements page #744

jcbcapps · 2022-08-16T15:28:41Z

SC-589

Proposed changes

Adds a carousel that features the four latest articles from the CMS on the news and announcements page

Reviewer notes

With at least one article published on your local CMS, navigate to /news-announcements

Verify that the carousel is at the top of the page, and shows the title and preview test of your article, along with a default CTA with the text "View Full Announcement" and a placeholder image
Verify that there is a button beneath the carousel with the text "View older Internal USSF news" and clicking that button takes you to /news where all CMS articles are displayed
Verify that clicking the CTA opens the article in another tab

Setup

Make sure to have both the CMS and the portal client running on your local machine. You can do this by

Running yarn services:up && yarn dev in the CMS repo and
Running yarn dev in the portal client with this branch checked out

Also, make sure that you have some published articles in your local CMS that have the category InternalNews

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

See for where the solution came from DefinitelyTyped/DefinitelyTyped#35572 (comment)

shortcut-integration · 2022-08-16T15:28:44Z

This pull request has been linked to Shortcut Story #589: Add an internal news carousel to News & Announcements page.

abbyoung · 2022-08-17T21:16:38Z

src/__fixtures__/data/cmsPortalNewsArticles.ts

@@ -1,38 +1,42 @@
 export const cmsPortalNewsArticlesMock = [
  {
    __typename: 'Article',
-    id: 'cl3buldda0247riyt6h85cpgc',
+    id: 'id1',


q: can you remind me why we needed to change these?

Oh it was just something I thought would be helpful. I've noticed in other mock data in the repo, we're using just basic ids (ex: 1, test1) so I just thought I'd make it less complicated. I copy/pasted the data from a query I did against my local cms, and I just thought all that long text is a pain to look at. Just thinking ahead to if anyone ever needs to differentiate between items in the array via the id or something it would just be quicker/easier to look at.

gotcha -- i think overall we can definitely improve our mock data and should align on some styles, like what should be as close to the format of prod data (the previous id) and what can be simplified (the current id). for a future eng sync, maybe! cc @gidjin

Aligning on some standards of when to pick a production format vs not. I could go either way here so probably worth us talking it out a little bit or just agreeing to one. Would / Could the library you were looking at for seed data help in this situation as well?

abbyoung · 2022-08-17T21:46:45Z

src/components/NewsCarousel/NewsCarousel.module.scss

+@import 'styles/sfds/sfdsDependencies';
+
+.carouselContainer {
+  display: flex !important;


might not need the !important? it renders correctly without it, but maybe there's an edge case i'm not seeing

Got it! Yeah there is a good chance that I just forgot to remove this.

abbyoung · 2022-08-17T21:51:33Z

src/components/NewsCarousel/NewsCarousel.module.scss

+    }
+
+    button::before {
+      color: transparent !important;


same as above -- the important for this button and the next don't seem to be necessary

abbyoung · 2022-08-17T22:07:21Z

src/components/NewsCarousel/NewsCarousel.stories.tsx

+  ],
+} as Meta
+
+const mockArticles: ArticleListItemRecord[] = [


TODO for future: Another opportunity to store/reuse data in __fixtures__/data

src/components/NewsCarousel/NewsCarousel.test.tsx

src/components/NewsCarouselItem/NewsCarouselItem.tsx

gidjin · 2022-08-18T00:34:20Z

src/pages/news-announcements.tsx

+      {articles.length > 0 && (
+        <section className={styles.newsCarouselSection}>
+          <NewsCarousel articles={articles} />
+        </section>
+      )}
+
+      {articles.length > 0 && (


Question: any reason we couldn't combine these two checks they look the same?

That's how I initially had it, but had to separate them because react-slick is using some styles (I think the position absolute) on the dots that was preventing me from getting any padding or margin between the two. I'm sure there is a work around we could do in CSS, but couldn't figure it out so thought this would be a passable solution.

Interesting. Might be worth a short comment to note that since it's not obvious from the code that was needed. Or maybe placing a <span/> in between them would solve it?

Didn't think about using a <span />. I'll give that a try and if it doesn't work, I'll add in a comment.

gidjin

code looks good, things work. I noticed something odd the styling of the carousel looks funny, (the image is too small and the layout is squished) if there is no preview filled in. Should we do something about that? Feels like preview would always be included but we don't mark it required so I'm not sure which way we should handle it. I think answering that can be part of a follow up story, so I'm approving.

gidjin · 2022-08-22T21:54:00Z

src/components/NewsCarousel/NewsCarousel.test.tsx


-    fireEvent.click(screen.getByTestId('slick-prev'))
-    expect(screen.getAllByText('Announcing the dev blog')).toHaveLength(2)
+    // TODO: verify that this actually changes the current slide in the carousel


praise: I'm sad this todo doesn't reflect the valiant effort to make it happen. So the praise is for that valiant effort and being willing to set it aside for now.

## [4.7.0](4.6.0...4.7.0) (2022-08-24) ### Features * Add internal news carousel for news and announcements page ([#744](#744)) ([c4acfe6](c4acfe6)), closes [/github.com/DefinitelyTyped/DefinitelyTyped/issues/35572#issuecomment-498242139](https://github.com/USSF-ORBIT//github.com/DefinitelyTyped/DefinitelyTyped/issues/35572/issues/issuecomment-498242139) * Display Spaceforce.mil news articles below the news carousel ([#746](#746)) ([e13b57a](e13b57a)), closes [/github.com/DefinitelyTyped/DefinitelyTyped/issues/35572#issuecomment-498242139](https://github.com/USSF-ORBIT//github.com/DefinitelyTyped/DefinitelyTyped/issues/35572/issues/issuecomment-498242139)

jcbcapps and others added 20 commits August 11, 2022 15:48

Change current news path to news-announcments

dfd2ad8

Update test

e3cecb4

Add NewsCarousel and NewsCarouselItem components

bd02b3d

Update storybook component

03cdf50

Add CTA

728db43

Add query for carousel articles

66672cc

Add NewsCarousel component to page

ba3009e

Add custom ellipses and custom arrows

c657ce9

Update styles

a902eff

Move class to module

077e442

fix: resolve type error from useRef return

e403e9c

See for where the solution came from DefinitelyTyped/DefinitelyTyped#35572 (comment)

test: add article mock to NewsAnnouncements component

b6520be

fix a11y violation in carousel, update mock in test

444466a

Style updates

aa417ab

Add test

afd2d18

Add test

684bc33

Add news page from main

c958f99

Add view older internal news link

823e7b4

Add comment

9b1914c

Update styles

67fd32b

jcbcapps requested review from abbyoung, esacteksab and gidjin as code owners August 16, 2022 15:28

jcbcapps and others added 2 commits August 16, 2022 10:31

Add conditional render for link

e037e7a

Merge branch 'main' into sc-589/internal-news-carousel

4de79e5

jcbcapps mentioned this pull request Aug 16, 2022

feat: Display Spaceforce.mil news articles below the news carousel #746

Merged

46 tasks

abbyoung reviewed Aug 17, 2022

View reviewed changes

gidjin reviewed Aug 18, 2022

View reviewed changes

Merge branch 'main' into sc-589/internal-news-carousel

39cde4b

jcbcapps requested a review from minhlai as a code owner August 18, 2022 14:24

jcbcapps and others added 4 commits August 18, 2022 09:52

Remove unnecessary importants

fd8bf16

Update default image

dd943d4

Update tests

034b37f

Merge branch 'main' into sc-589/internal-news-carousel

82989b8

gidjin approved these changes Aug 22, 2022

View reviewed changes

jcbcapps and others added 2 commits August 23, 2022 09:33

Merge branch 'main' into sc-589/internal-news-carousel

4d98c22

Remove flex styles

ff84f0e

abbyoung approved these changes Aug 23, 2022

View reviewed changes

Add comment

679c517

jcbcapps merged commit c4acfe6 into main Aug 23, 2022

jcbcapps deleted the sc-589/internal-news-carousel branch August 23, 2022 19:12

gidjin mentioned this pull request Aug 24, 2022

chore(release): 4.7.0 #758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add internal news carousel for news and announcements page #744

feat: Add internal news carousel for news and announcements page #744

jcbcapps commented Aug 16, 2022 •

edited

shortcut-integration bot commented Aug 16, 2022

abbyoung Aug 17, 2022

jcbcapps Aug 17, 2022 •

edited

abbyoung Aug 17, 2022

gidjin Aug 22, 2022

abbyoung Aug 17, 2022

jcbcapps Aug 18, 2022

abbyoung Aug 17, 2022

abbyoung Aug 17, 2022

gidjin Aug 18, 2022

jcbcapps Aug 18, 2022

gidjin Aug 22, 2022

jcbcapps Aug 23, 2022

gidjin left a comment

gidjin Aug 22, 2022

feat: Add internal news carousel for news and announcements page #744

feat: Add internal news carousel for news and announcements page #744

Conversation

jcbcapps commented Aug 16, 2022 • edited

SC-589

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

shortcut-integration bot commented Aug 16, 2022

Choose a reason for hiding this comment

jcbcapps Aug 17, 2022 • edited

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

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

gidjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbcapps commented Aug 16, 2022 •

edited

jcbcapps Aug 17, 2022 •

edited