ref(contactus): stylings for contact us page editor #1458

dcshzj · 2023-08-29T09:41:50Z

Problem

Closes IS-474.

Solution

Breaking Changes

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

Improvements:

The contact us page editor has been refactored to make use of the new stylings. This is placed behind a feature flag for now.

Before & After Screenshots

BEFORE:

AFTER:

Tests

Deploy Notes

None

seaerchin

my main issues are that:

the changes in EditContactUs are q big and it isn't immediately obvious why it changed so idk if functionality is still preserved :monkas:
not entirely sure if the cast is safe - this might lead to breakage. i'm ok w the cast itself, just that because fields can be undefined, the cast needs to prove that the required fields always exist

seaerchin · 2023-08-29T10:00:05Z

src/layouts/LegacyEditContactUs.jsx

actually very tempted to put in folder but ok... i'll take the L

src/utils/validators.js

src/layouts/FeatureFlaggedContactUs.tsx

src/layouts/EditContactUs.jsx

seaerchin · 2023-08-29T10:17:22Z

src/utils/validators.js

+        }
+      } else if (_.startsWith(strippedValue, "1800")) {
+        if (
+          !strippedValue.includes("_") &&


sanity check here, should this not be -?

Hmm no, this is an extension of the check just above where it checks if the strippedValue includes _, so it should remain as _.

wait do international numbers use _? i always thought was - (based off tv + movie ._.)

ok with this as-is cos it's existing behaviour, fix can come later.

seaerchin · 2023-08-29T10:18:34Z

src/utils/validators.js

      }
      break
    }
    case "maps_link": {
+      if (value.length > 0 && !value.startsWith("https://")) {


should we also allow http || www?

For www we should not, because it will not correctly format the link for the end-user, so we definitely need to specify the protocol. For http, I decided that generally since our sites are in HTTPS, we should also make links go to other HTTPS sites as much as possible. At least for Google Maps and OneMap, they are in HTTPS so this should not affect them.

to double check - this is new functionality that wasn't previously present right? i actually don't mind this (and i agree w/ your reasoning here), just wanted to confirm this

seaerchin · 2023-08-30T03:29:31Z

fyi - didn't run manual tests, will do so tmrw. pls tell me that you did actually run the manual tests on local :monkas:

dcshzj · 2023-08-30T04:37:01Z

fyi - didn't run manual tests, will do so tmrw. pls tell me that you did actually run the manual tests on local :monkas:

Yup!

dcshzj · 2023-08-30T04:51:27Z

the changes in EditContactUs are q big and it isn't immediately obvious why it changed so idk if functionality is still preserved :monkas:

The changes can be summarised into 2 key changes:

Styling changes (these are the changes within the hasLoaded condition) - this is definitely necessary since it is the main premise of the issue/PR
onDragEnd enhancements - this is needed to support the added functionality provided by the nested drag-and-drop within the locations section

not entirely sure if the cast is safe - this might lead to breakage. i'm ok w the cast itself, just that because fields can be undefined, the cast needs to prove that the required fields always exist

I removed most of the unnecessary casts after shifting to use the EditableContext. The ones remaining are safe:

Casting to either LocationCardFrontMatter or ContactCardFrontMatter is ok since it is only narrowing from the parent type, which is the union of both types
Casting to PhonePrefix is ok since the event.target.value provided is obtained from the value of the dropdown options, which is generated from PhonePrefix.

seaerchin · 2023-08-30T05:18:31Z

from offline discussion

onDragEnd enhancements - this is needed to support the added functionality provided by the nested drag-and-drop within the locations section

we are implicitly passing the parentIndex using the id of the element for now. this is safe because the changes are mostly copied over from existing code + an additional indexing step to get the correct parent index.

the draggableId for locationCards is set only in 1 file LocationCard so this is also limited in scope for now.

having said that, i think a possible enhancement for maintainability would be to pass the index explicitly via a wrapper - this would make things clearer and reduce mental load + correctness checking

seaerchin

lgtm

* docs(editable): add docs for drag drop stuf (#1445) Co-authored-by: seaerchin <[email protected]> * fix(editable): shift out contxt to allow nested dnd (#1448) Co-authored-by: seaerchin <[email protected]> * feat: introduce new version of NotFoundPage (#1435) * Add submarine image for NotFoundPage * Add new version of the NotFoundPage * Add storybook stories for new NotFoundPage * Fix unit test for RouteSelector * fix(ui): changes based on design feedback * IS-322 Setup GrowthBook for FE (#1449) * feat: add growthbook as dep * feat: add growthbook to app * feat: inject user attributes * feat: add default dev mode to be false * feat: inject site name as attribute * feat: add types and featur flag list * chore: minor fixes * feat: add type and conditional siteName attribute * fix: enable dev mode only for local dev * chore: remove duplicate comment * IS-417 - Fixing Selector Issues in E2E tests (#1453) * feat: update text assertion * fix: resources text assertion * fix: method to remove other collaborators * fix: uncomment code * feat: extract constant * feat: swap removeFirstCollaborator with removeOtherCollaborators * fix: use removeOtherCollaborators * chore: remove unused import and function * chore(edithomepage): shift constants out (#1416) * refactor(edithomepage): use new constants * refactor(edithomepage): shift to new folder * refactor(edithomepage): shift preview out into own section --------- Co-authored-by: seaerchin <[email protected]> * IS-508: Introduce Interface for GrowthBook (#1455) * feat: introduce interface, remove constants file * fix: comment out sample flags * ref(contactus): stylings for contact us page editor (#1458) * Enhance AddSectionButton to allow adjusting button text * Add various sections of contact us page * Adjust Editable to allow new droppable zones * Update validators to match Figma messages * Introduce feature-flagged contact us page * Remove unneeded comment * Switch to use editable contexts instead and use modal * Fix validation condition * Fix name for FeatureFlaggedContactUs * Remove some unnecessary casts * Fix RouteSelector unit test * chore: set pointerEvents to none for draggable icon button (#1456) * Feat/is 443 nav bar styling (#1457) * chore: update editable types to allow nav * feat: add pageMenuBody * feat: add resourceMenuBody * feat: add folderMenuBody * feat: add GroupMenuBody * feat: swap new components into editNavBar * chore: fix text for components * Fix: addSectionButton button text prop * chore: pass props to button instead of box * chore: add feature flag for new nav bar (#1459) * chore: add feature flag for new nav bar * fix: tests * feat(feature-flag): add feature flag from gb (#1462) * feat(feature-flag): add feature flag from gb * refactor(navbar): use gb for ff * refactor(feature-flag): add types --------- Co-authored-by: seaerchin <[email protected]> * fix(edithomepage): remove optional tags (#1450) Co-authored-by: seaerchin <[email protected]> * Feat/is 494 smooth scroll (#1439) * refactor(edithomepage): smooth scroll * fix: only scroll after first load * chore: fix rebase errors * Revert "fix: only scroll after first load" This reverts commit 158a4df. * Revert "refactor(edithomepage): smooth scroll" This reverts commit e555c2f. * fix: firstLoad condition * chore: move first load check to separate method * fix: smooth scroll --------- Co-authored-by: seaerchin <[email protected]> Co-authored-by: Alexander Lee <[email protected]> * Fix/homepage images on private repos (#1461) * chore: add util method to retrieve image path * fix: replace image retrieval in homepage template components with updated v2 endpoint * fix: change return of util method to handle nested * Update src/utils/images.ts Co-authored-by: Qilu Xie <[email protected]> * Update src/utils/images.ts Co-authored-by: Qilu Xie <[email protected]> * chore: fix styling * refactor: move image fetching to separate hook --------- Co-authored-by: Qilu Xie <[email protected]> * 0.41.0 --------- Co-authored-by: seaerchin <[email protected]> Co-authored-by: seaerchin <[email protected]> Co-authored-by: Harish <[email protected]> Co-authored-by: Hsu Zhong Jun <[email protected]> Co-authored-by: Alexander Lee <[email protected]> Co-authored-by: Qilu Xie <[email protected]>

dcshzj added 5 commits August 29, 2023 17:20

Enhance AddSectionButton to allow adjusting button text

a8fa436

Add various sections of contact us page

66dda6a

Adjust Editable to allow new droppable zones

bc2bf3e

Update validators to match Figma messages

b69f64b

Introduce feature-flagged contact us page

e235629

dcshzj requested a review from a team August 29, 2023 09:41

Remove unneeded comment

14a0bf2

dcshzj temporarily deployed to staging August 29, 2023 09:43 — with GitHub Actions Inactive

dcshzj temporarily deployed to staging August 29, 2023 10:00 — with GitHub Actions Inactive

Switch to use editable contexts instead and use modal

c27d54f

dcshzj temporarily deployed to staging August 30, 2023 02:40 — with GitHub Actions Inactive

dcshzj temporarily deployed to staging August 30, 2023 02:55 — with GitHub Actions Inactive

seaerchin reviewed Aug 30, 2023

View reviewed changes

dcshzj added 2 commits August 30, 2023 12:31

Fix validation condition

4ddeacd

Fix name for FeatureFlaggedContactUs

d33392d

dcshzj temporarily deployed to staging August 30, 2023 04:32 — with GitHub Actions Inactive

dcshzj had a problem deploying to staging August 30, 2023 04:44 — with GitHub Actions Failure

Remove some unnecessary casts

c908fc2

dcshzj temporarily deployed to staging August 30, 2023 04:45 — with GitHub Actions Inactive

dcshzj had a problem deploying to staging August 30, 2023 04:57 — with GitHub Actions Failure

seaerchin approved these changes Aug 30, 2023

View reviewed changes

Fix RouteSelector unit test

e2e9682

dcshzj temporarily deployed to staging August 30, 2023 05:26 — with GitHub Actions Inactive

dcshzj temporarily deployed to staging August 30, 2023 05:40 — with GitHub Actions Inactive

seaerchin merged commit 4f03e6f into develop Aug 30, 2023

seaerchin deleted the IS-474-contact-us-styling branch August 30, 2023 06:15

This was referenced Aug 30, 2023

release(0.41.0): merge to develop #1463

Merged

release(0.41.0): merge to prod #1464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(contactus): stylings for contact us page editor #1458

ref(contactus): stylings for contact us page editor #1458

dcshzj commented Aug 29, 2023

seaerchin left a comment

seaerchin Aug 29, 2023

seaerchin Aug 29, 2023

dcshzj Aug 30, 2023

seaerchin Aug 30, 2023

seaerchin Aug 30, 2023

seaerchin Aug 29, 2023

dcshzj Aug 30, 2023

seaerchin Aug 30, 2023

seaerchin commented Aug 30, 2023

dcshzj commented Aug 30, 2023

dcshzj commented Aug 30, 2023

seaerchin commented Aug 30, 2023

seaerchin left a comment

ref(contactus): stylings for contact us page editor #1458

ref(contactus): stylings for contact us page editor #1458

Conversation

dcshzj commented Aug 29, 2023

Problem

Solution

Before & After Screenshots

Tests

Deploy Notes

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

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 commented Aug 30, 2023

dcshzj commented Aug 30, 2023

dcshzj commented Aug 30, 2023

seaerchin commented Aug 30, 2023

seaerchin left a comment

Choose a reason for hiding this comment