AAA I can set an announcement with funnels and CTA button #1440

oleksii-front · 2021-05-25T06:57:50Z

closes https://github.com/sparkletown/internal-sparkle-issues/issues/744

src/components/organisms/BannerAdmin/BannerAdmin.tsx

codeclimate · 2021-05-25T06:59:17Z

Code Climate has analyzed commit 4c96067 and detected 7 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2
Duplication	5

View more on Code Climate.

github-actions · 2021-05-25T07:02:41Z

Visit the preview URL for this PR (updated for commit 1223c52):

https://co-reality-staging--preview-pr-1440-9i5xawm9.web.app

_{(expires Fri, 01 Oct 2021 10:55:13 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

mike-lvov

First pass of the review. Overall looks good. Thanks for the refactors!

src/api/bannerAdmin.ts

src/assets/icons/icon-checkmark.svg

src/components/molecules/AnnouncementMessage/AnnouncementMessage.scss

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

src/pages/Admin/Venue/AnnouncementOptions/AnnouoncementStatus/AnnouncementStatus.tsx

src/pages/Admin/Venue/VenueAdminPage.scss

src/pages/VenuePage/TemplateWrapper.tsx

src/types/banner.ts

0xdevalias

Comments/questions/changes/etc. May need someone else like @sunny-viktoryia or similar to handle follow on reviews/etc, though I will try and keep an eye out for responses to my questions/etc.

Also, seems this PR has merge conflicts.

functions/venue.js

src/api/bannerAdmin.ts

src/components/atoms/LinkButton/LinkButton.tsx

src/components/molecules/AnnouncementMessage/AnnouncementMessage.scss

src/pages/Admin/Venue/VenueAdminPage.tsx

0xdevalias · 2021-06-25T05:38:42Z

src/pages/Admin/Venue/VenueAdminPage.tsx

-    <>
-      <h4 className="admin-page-title">You are editing venue: {venue.name}</h4>
-      <BannerAdmin venueId={venueId} venue={venue} />
-      {isIframeVenue && <IframeAdmin venueId={venueId} venue={venue} />}


Why was IframeAdmin removed from here? That doesn't seem right..?

seems to be removed in the design as well https://www.figma.com/file/oX3Px4vYjjUkOZYSqcCgWI/Sparkle-Banner-Announcements?node-id=3%3A6

Maybe sync with @sparkletown/product and find out the ‘why’ of that, if it was explicitly intended. I have a feeling it may be a case of the designs not reflecting the reality of the codebase rather than an explicit desire

@0xdevalias as per discussion with Sofi this banner feature is to be replaced by announcement one, so that should explain the removal of this code piece

IframeAdmin is entirely different to banneradmin..

@Alexey-Petrov, please, get <IframeAdmin /> back. It's about changing the URL for iframe venues.

src/pages/Admin/Venue/VenueAdminPage.tsx

src/types/banner.ts

src/pages/VenuePage/TemplateWrapper.tsx

sunny-viktoryia · 2021-06-25T10:36:51Z

May need someone else like @sunny-viktoryia or similar to handle follow on reviews/etc, though I will try and keep an eye out for responses to my questions/etc.

@0xdevalias yes, can take it. 🙃
@oleksii-front, please ping me when I can re-review it.

sunny-viktoryia

another round of review. 🙃

src/components/atoms/ConfirmationModal/ConfirmationModal.tsx

src/components/atoms/ConfirmationModal/ConfirmationModal.scss

src/components/organisms/BannerAdmin/BannerAdmin.tsx

src/components/atoms/ConfirmationModal/ConfirmationModal.tsx

src/pages/Admin/Venue/AnnouncementOptions/AnnouncementOptions.scss

sunny-viktoryia · 2021-09-22T16:06:42Z

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

+        )}
+
+        <div className="AnnouncementMessage__content">
+          <RenderMarkdown text={banner?.content} />


but you have useEffect which handles not defined state.

useEffect(() => { if (isDefined(banner?.content)) { showAnnouncementMessage(); } }, [banner, showAnnouncementMessage]);

and then you do

if (!isAnnouncementMessageVisible) return null;

…/banner-announcement

alexxpetrov · 2021-09-23T12:27:48Z

@sunny-viktoryia Updated general view according to latest comments in https://github.com/sparkletown/internal-sparkle-issues/issues/744
so there are some new changes including fixes according to your comments

sunny-viktoryia

looks like it's close to getting merged. Found a bug. Please, check the comments. 🙃

src/components/atoms/ConfirmationModal/ConfirmationModal.tsx

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

src/components/atoms/ConfirmationModal/ConfirmationModal.tsx

sunny-viktoryia · 2021-09-23T16:02:41Z

src/components/atoms/ConfirmationModal/ConfirmationModal.tsx

      <Modal.Body>
-        <div>
-          {hasHeader && <Modal.Title>{header}</Modal.Title>}
+        <div className="ConfirmationModal">


why the class of the ConfirmationModal is moved? it should be set on the container element.

it's still a container class for the confirmation content body

well, let's say we want to change the background of the confirmation modal. If we change it for the current ".ConfirmationModal" selector, it'll look this way, leaving black borders (see the screenshot). But if we assign this class name to the Modal component, we'll be able to target a child to change the background of the needed element.

sunny-viktoryia · 2021-09-23T16:11:22Z

src/components/organisms/BannerAdmin/BannerAdmin.scss

+  }
+}
+
+.BannerAdminModal {


do we have this class name now?

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

sunny-viktoryia · 2021-09-23T16:14:02Z

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

@@ -14,42 +14,42 @@ import { LinkButton } from "components/atoms/LinkButton";
 import "./AnnouncementMessage.scss";

 export interface AnnouncementMessageProps {
-  banner?: Banner;


why was that changed? looks like we could get banner from everywhere without the need to get it here manually,

I suppose it would be better to fetch banner info inside the banner component itself, so that it doesn't rely on the parent component

sunny-viktoryia · 2021-09-23T16:15:17Z

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

+  const venueId = useVenueId();
+  const { currentVenue: venue } = useConnectCurrentVenueNG(venueId);
+
+  const { banner } = venue ?? {};


you can use ALWAYS_EMPTY_OBJECT here instead of {}.

doesn't work in this case for some reason

…/banner-announcement

sunny-viktoryia

Approving. Thanks for your efforts! ❤️ It was a loooooong way!
Although would be perfect to address the comments to the review before the merge as well.

and a question: should the announcement hide when I scroll the venue?

src/components/molecules/AnnouncementMessage/AnnouncementMessage.tsx

sunny-viktoryia · 2021-09-24T13:54:02Z

src/components/atoms/ConfirmationModal/ConfirmationModal.tsx

      <Modal.Body>
-        <div>
-          {hasHeader && <Modal.Title>{header}</Modal.Title>}
+        <div className="ConfirmationModal">


well, let's say we want to change the background of the confirmation modal. If we change it for the current ".ConfirmationModal" selector, it'll look this way, leaving black borders (see the screenshot). But if we assign this class name to the Modal component, we'll be able to target a child to change the background of the needed element.

…/banner-announcement

dismissing as per #1440 (review)

…/banner-announcement

oleksii-front requested a review from a team as a code owner May 25, 2021 06:57

oleksii-front self-assigned this May 25, 2021

oleksii-front temporarily deployed to feature-preview May 25, 2021 06:57 Inactive

oleksii-front added the 🚧 WIP Work In Progress (consider making the PR a draft instead of/as well as using this label) label May 25, 2021

github-actions bot added the 💎 styles For (S)CSS style related issues/changes/improvements/etc. label May 25, 2021

codeclimate bot reviewed May 25, 2021

View reviewed changes

oleksii-front removed the 🚧 WIP Work In Progress (consider making the PR a draft instead of/as well as using this label) label May 25, 2021

oleksii-front requested review from 0xdevalias and mike-lvov May 25, 2021 14:46

oleksii-front temporarily deployed to feature-preview May 28, 2021 15:38 Inactive

oleksii-front temporarily deployed to feature-preview May 31, 2021 10:37 Inactive

oleksii-front temporarily deployed to feature-preview May 31, 2021 11:10 Inactive

oleksii-front temporarily deployed to feature-preview June 3, 2021 08:19 Inactive

mike-lvov previously requested changes Jun 3, 2021

View reviewed changes

oleksii-front requested a review from mike-lvov June 13, 2021 10:30

0xdevalias previously requested changes Jun 25, 2021

View reviewed changes

0xdevalias added the 💪 enhancement Enhancements/improvements to existing functionality label Jun 25, 2021

sunny-viktoryia self-requested a review June 25, 2021 10:15

oleksii-front temporarily deployed to feature-preview June 25, 2021 15:23 Inactive

oleksii-front requested a review from 0xdevalias June 28, 2021 14:55

alexxpetrov assigned alexxpetrov and unassigned oleksii-front Jul 1, 2021

alexxpetrov force-pushed the feature/banner-announcement branch from 0eccd9d to 771b70a Compare July 1, 2021 10:45

alexxpetrov temporarily deployed to feature-preview July 1, 2021 10:46 Inactive

alexxpetrov temporarily deployed to feature-preview July 2, 2021 11:54 Inactive

alexxpetrov temporarily deployed to feature-preview July 2, 2021 12:52 Inactive

alexxpetrov force-pushed the feature/banner-announcement branch from 9937587 to 2824976 Compare July 2, 2021 12:53

sunny-viktoryia reviewed Sep 22, 2021

View reviewed changes

Oleksii Petrov added 2 commits September 23, 2021 15:12

mr fixes + announcement preview + ui redone

d93f7db

Merge branch 'staging' of github.com:sparkletown/sparkle into feature…

b0a1908

…/banner-announcement

alexxpetrov temporarily deployed to feature-preview September 23, 2021 12:14 Inactive

removed unnecessary classes

f4d50ec

alexxpetrov temporarily deployed to feature-preview September 23, 2021 12:25 Inactive

Merge branch 'staging' of github.com:sparkletown/sparkle into feature…

cc3ea03

…/banner-announcement

alexxpetrov temporarily deployed to feature-preview September 23, 2021 12:27 Inactive

fix height navbar

059ddbc

alexxpetrov temporarily deployed to feature-preview September 23, 2021 13:04 Inactive

sunny-viktoryia reviewed Sep 23, 2021

View reviewed changes

Oleksii Petrov added 2 commits September 24, 2021 12:20

Merge branch 'staging' of github.com:sparkletown/sparkle into feature…

515bb0d

…/banner-announcement

mr fixes + announcement position fix for admin

a4e73af

alexxpetrov temporarily deployed to feature-preview September 24, 2021 10:47 Inactive

Merge branch 'staging' of github.com:sparkletown/sparkle into feature…

1223c52

…/banner-announcement

alexxpetrov temporarily deployed to feature-preview September 24, 2021 10:50 Inactive

sunny-viktoryia approved these changes Sep 24, 2021

View reviewed changes

minor fixes

bf4ee63

alexxpetrov had a problem deploying to feature-preview September 24, 2021 14:20 Failure

Merge branch 'staging' of github.com:sparkletown/sparkle into feature…

271ce1e

…/banner-announcement

alexxpetrov had a problem deploying to feature-preview September 24, 2021 14:22 Failure

Merge branch 'staging' of github.com:sparkletown/sparkle into feature…

4c96067

…/banner-announcement

alexxpetrov had a problem deploying to feature-preview September 24, 2021 15:59 Failure

alexxpetrov enabled auto-merge (squash) September 24, 2021 15:59

alexxpetrov merged commit 0493abd into staging Sep 24, 2021

alexxpetrov deleted the feature/banner-announcement branch September 24, 2021 16:01

AAA I can set an announcement with funnels and CTA button #1440

AAA I can set an announcement with funnels and CTA button #1440

Uh oh!

Conversation

oleksii-front commented May 25, 2021 • edited by sunny-viktoryia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codeclimate bot commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mike-lvov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xdevalias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sunny-viktoryia commented Jun 25, 2021

Uh oh!

sunny-viktoryia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexxpetrov commented Sep 23, 2021

Uh oh!

sunny-viktoryia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleksii-front commented May 25, 2021 •

edited by sunny-viktoryia

Loading

codeclimate bot commented May 25, 2021 •

edited

Loading

github-actions bot commented May 25, 2021 •

edited

Loading