Feat/announcement block #1497

kishore03109 · 2023-09-14T09:01:51Z

Problem

Announcement block editing in homepage according to figma.

Solution

In the interest of consistency, similar approach to key_highlights in the hero banner.

Breaking Changes

Yes - this PR contains breaking changes
- Details ...
No - this PR is backwards compatible

Screen.Recording.2023-09-15.at.7.10.16.PM.mov

Tests

Go into edit homepage
Click on add new announcement
Ensure that when new announcements are created, they are created from the top
Ensure that save button is not available when errors are present
Ensure that when there are errors in the announcement item there is a red outline in both
1. The annoucemnet item
2. The entire announcement block
You can drag and drop announcements
When one announcement block is clicked, you cannot add more announcement blocks (only allowed <= 1 announcement block)
You can only add up to 5 announcement items (only allowed <= 5 announcement items)
When save is clicked + page is refreshed, the announcement persists -> suggests that the changes were sucessfully published to gh.

Known limitations

Announcement does not open up on create. (I attempted to use displayAnnouncements for this but somehow it is not working as intended. Filing a ticket here.)
Implementing unique keys in AnnouncementBody.tsx. Since we are rendering components over a map, each element would require a unique key. However, when I did do it the re-renders were visible + triggered an off-focus 😱. Have anyone solved this problem before?
.
This key was removed in 1556850

Blockers

I think it is difficult for design to iterate give a green check here since we don't have storybook. I tried (a timeboxed) attempt it b5b41cc but the paddings and all were off so this attempt does not yield the benefits that I was hoping for. Since this to be shipped soon, for now Ill document the flows in video form to get Design's [@sehyunidaaa] approval first.

Note to reviewers

This PR is too bulky for my taste, I am seeking feedback on how I could have split this PR up (or if this PR size is actually ok).

kishore03109 · 2023-09-15T11:27:29Z

src/utils/validators.js

@@ -803,13 +915,6 @@ const validateDayOfMonth = (month, day) => {
  }
 }

-const validateNonFutureDate = (dateStr) => {


Note: this is not removed, but shifted up + renamed to group similar functions tgt

dcshzj

Not a complete review yet, left EditHomepage.jsx but have some concerns regarding frontmatter

src/utils/validators.js

src/types/homepage.ts

src/layouts/EditHomepage/EditHomepage.jsx

src/layouts/components/Homepage/AnnouncementBody.tsx

dcshzj · 2023-09-18T10:04:01Z

src/layouts/components/Homepage/AnnouncementBody.tsx

+                    {
+                      title: announcementTitle,
+                      date: announcementDate,
+                      announcement: announcementContent,
+                      link_text: announcementLinkText,
+                      link_url: announcementLinkUrl,
+                    },


Not a functional comment but just wondering if it would be better to just have announcement object here, then call things like announcement.title below.

hey i rather destructure at the top bah.
i didnt want to do announcement.announcement for clarity

src/layouts/components/Homepage/AnnouncementBody.tsx

src/layouts/EditHomepage/EditHomepage.jsx

src/hooks/useDrag.tsx

src/layouts/EditHomepage/EditHomepage.jsx

seaerchin · 2023-09-20T05:44:25Z

src/layouts/components/Homepage/AnnouncementBody.tsx

+                              id={`announcements-${announcementIndex}-date`}
+                              inputValue={announcementDate}
+                              onInputValueChange={(value) => {
+                                onChange({


feels like i shuold've wrapped the onChange ._.

seaerchin · 2023-09-20T05:45:02Z

src/layouts/components/Homepage/AnnouncementBody.tsx

skimmed - seems to be ok

seaerchin · 2023-09-20T05:47:35Z

src/templates/homepage/AnnouncementsSection.tsx

+                                {moment(announcement.date, "DD/MM/YYYY").format(
+                                  "DD MMMM YYYY"
+                                )}


noted that this will reconcile the diff in view but it still feels weird to have it here (as a view level thing) rather than just doing it at the data level. this offloads responsibility to the dev next time to know this @__@

src/utils/validators.js

seaerchin · 2023-09-20T05:49:24Z

src/utils/validators.js

@@ -218,6 +240,57 @@ const validateHighlights = (highlightError, field, value) => {
  return newHighlightError
 }

+const validateAnnouncementItems = (announcementError, field, value) => {


could we separate out the validation from the set? this makes it easier to reuse in the future, which was 1 blocker i faced when trying to use the existing validators

validation from the set? May I get your help to expand more on this/direct me to the part of the codebase that uses the pattern you are trying to explain?

abit unclear wording on my end, but validateAnnouncementItems sets the corresponding index. in this case, it does not simply validate (ie, return bool + msg) but also does a side effect (setting the corresponding index). this isn't particularly important to fix now because it follows the existing style of code but when we want to do simple validation that is unaware of the structure of the error, will be an issue

hmm ok, am inclined to keep it as is for consistency with the rest of the local code, will modify if future pain points warrant it?

seaerchin

mostly sanity checks

seaerchin · 2023-09-21T06:14:47Z

src/layouts/EditHomepage/EditHomepage.jsx

+        let {
          content: { frontMatter },
-          sha,
        } = homepageData
+        const { sha } = homepageData


nit: this change appears to have different semantics; content now appears to be writable now whereas previously it was only readable

Hey yes this is the case. There was a bug when the key was undefined, but its still valid frontmatter state. As such, I just refine an empty array so that our FE does not throw any error.

src/layouts/EditHomepage/EditHomepage.jsx

src/layouts/EditHomepage/constants.ts

src/layouts/components/Homepage/AnnouncementBody.tsx

src/types/homepage.ts

src/layouts/components/Homepage/InfobarBody.tsx

src/layouts/components/Homepage/AnnouncementBody.tsx

kishore03109 · 2023-09-21T12:28:49Z

@seaerchin ps need to your look at small additional commit (the others shd be trivial)
8c46a2c

kishore03109 temporarily deployed to staging September 14, 2023 09:13 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging September 14, 2023 09:15 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging September 15, 2023 06:39 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging September 15, 2023 07:07 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging September 15, 2023 08:30 — with GitHub Actions Inactive

kishore03109 marked this pull request as ready for review September 15, 2023 11:15

kishore03109 requested a review from a team September 15, 2023 11:16

kishore03109 temporarily deployed to staging September 15, 2023 11:20 — with GitHub Actions Inactive

kishore03109 commented Sep 15, 2023

View reviewed changes

dcshzj reviewed Sep 18, 2023

View reviewed changes

kishore03109 temporarily deployed to staging September 18, 2023 10:32 — with GitHub Actions Inactive

dcshzj reviewed Sep 19, 2023

View reviewed changes

src/layouts/EditHomepage/EditHomepage.jsx Outdated Show resolved Hide resolved

src/layouts/EditHomepage/EditHomepage.jsx Show resolved Hide resolved

src/layouts/EditHomepage/EditHomepage.jsx Show resolved Hide resolved

kishore03109 temporarily deployed to staging September 19, 2023 04:51 — with GitHub Actions Inactive

kishore03109 requested a review from dcshzj September 19, 2023 07:38

kishore03109 temporarily deployed to staging September 19, 2023 07:57 — with GitHub Actions Inactive

dcshzj force-pushed the IS-517-announcements-previews branch from 7601bf0 to 94e3af7 Compare September 19, 2023 12:13

kishore03109 force-pushed the feat/announcementBlock branch from dc75957 to 9c9b398 Compare September 20, 2023 05:11

kishore03109 temporarily deployed to staging September 20, 2023 05:24 — with GitHub Actions Inactive

seaerchin reviewed Sep 20, 2023

View reviewed changes

kishore03109 temporarily deployed to staging September 20, 2023 10:28 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging September 20, 2023 10:45 — with GitHub Actions Inactive

kishore03109 requested a review from seaerchin September 20, 2023 12:06

kishore03109 temporarily deployed to staging September 21, 2023 01:35 — with GitHub Actions Inactive

kishore03109 changed the base branch from IS-517-announcements-previews to develop September 21, 2023 03:29

kishore03109 force-pushed the feat/announcementBlock branch from 667c783 to 01e35b9 Compare September 21, 2023 03:37

kishore03109 temporarily deployed to staging September 21, 2023 03:54 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging September 21, 2023 04:11 — with GitHub Actions Inactive

seaerchin reviewed Sep 21, 2023

View reviewed changes

src/layouts/components/Homepage/AnnouncementBody.tsx Outdated Show resolved Hide resolved

kishore03109 temporarily deployed to staging September 21, 2023 07:17 — with GitHub Actions Inactive

kishore03109 added 20 commits September 21, 2023 20:26

refactor(announcement): mutate at data level

73bfcab

fix(announcement body): copy changes

f3479d7

fix(announcement): copy changes for clarity

87c4b03

fix(announcement date): show invalid dates

6f1a855

style(copy): change copy

cc124cd

fix(announcement): copy changes

5b1ddc1

fix(announcement): have 1 announcemnt by default

545e9b8

build(de): fix failing build

38b165a

fix(homapage prevew): rebase errors

f395799

chore(validators): rm console logs

2c77e9e

feat(feature flag): add partial ff for announment

398e866

chore(announcemnt body): rm console log

c3b8bae

chore(announcementBody): comments for clarity

8d6940e

fix(typing): relax constraint

700d1d2

chore(announcementbody): rm redundant console.log

c99a882

refactor(editHomepage): rm redundant spread

d444b96

style(edit homepage): add comments for clarity

b932615

fix(announcements): empty errors bug

8c46a2c

chore(edit homepage): rm redundant func call

12fb497

chore(edit homepage): clarify comment

612aba0

kishore03109 force-pushed the feat/announcementBlock branch from c60139a to 612aba0 Compare September 21, 2023 12:27

kishore03109 added 2 commits September 22, 2023 09:42

fix(imports): fix imports after rebase

1dc61cc

fix(imports): issues after rebase

b13307c

kishore03109 temporarily deployed to staging September 22, 2023 04:01 — with GitHub Actions Inactive

seaerchin approved these changes Sep 22, 2023

View reviewed changes

kishore03109 merged commit c9294c7 into develop Sep 22, 2023
6 checks passed

mergify bot deleted the feat/announcementBlock branch September 22, 2023 08:40

This was referenced Sep 25, 2023

release(0.45.0): merge to develop #1519

Merged

release(v0.45.0): merge to master #1520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/announcement block #1497

Feat/announcement block #1497

kishore03109 commented Sep 14, 2023 •

edited by seaerchin

Loading

kishore03109 Sep 15, 2023

dcshzj left a comment

dcshzj Sep 18, 2023

kishore03109 Sep 19, 2023

seaerchin Sep 20, 2023

seaerchin Sep 20, 2023

seaerchin Sep 20, 2023

kishore03109 Sep 20, 2023

seaerchin Sep 20, 2023

kishore03109 Sep 20, 2023

seaerchin Sep 21, 2023

kishore03109 Sep 21, 2023

seaerchin left a comment

seaerchin Sep 21, 2023

kishore03109 Sep 21, 2023

kishore03109 commented Sep 21, 2023

Feat/announcement block #1497

Feat/announcement block #1497

Conversation

kishore03109 commented Sep 14, 2023 • edited by seaerchin Loading

Problem

Solution

Tests

Known limitations

Blockers

Note to reviewers

Choose a reason for hiding this comment

dcshzj 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

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

seaerchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 commented Sep 21, 2023

kishore03109 commented Sep 14, 2023 •

edited by seaerchin

Loading