feat: Newsletter Banner #601
Conversation
client/index.html
Outdated
| id="hubspot-js" | ||
| type="text/javascript" | ||
| src="https://js.hsforms.net/forms/v2.js" | ||
| ></script> |
There was a problem hiding this comment.
This is to use Hubspot's JS file to embed their form and submit the email to their API
| @@ -96,9 +95,6 @@ class App extends React.Component<Props> { | |||
| viewportRef={viewportRef} | |||
| key={graphRenderCounter} | |||
| /> | |||
| <Controls bottom={0}> | |||
There was a problem hiding this comment.
We're moving the breadcrumbs into the component for finer control of the styling based on if the banner is open or not
|
Example: explorer.newsletter.changes.movNote that the header/nav bar, breadcrumbs, graphs, and gene sets are placeholder values to mimic how actual Explorer would look from DP and to demonstrate scrolling of the left/right sidebars |
Codecov Report
@@ Coverage Diff @@
## main #601 +/- ##
=======================================
Coverage 77.67% 77.67%
=======================================
Files 88 88
Lines 6754 6754
=======================================
Hits 5246 5246
Misses 1508 1508
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
client/src/global.d.ts
Outdated
There was a problem hiding this comment.
Can be ignored, just configuring types from the Hubspot JS
There was a problem hiding this comment.
Can be ignored, just styling for the newsletter modal and banner
| globals.rightSidebarWidth + 1 | ||
| }px [right-sidebar-end] | ||
| `, | ||
| [left-sidebar-start] ${globals.leftSidebarWidth + 1}px |
There was a problem hiding this comment.
just indent changes
| @@ -57,6 +62,7 @@ const Layout: React.FC<Props> = (props) => { | |||
| position: "relative", | |||
| height: "inherit", | |||
| overflowY: "auto", | |||
| paddingBottom: isBannerOpen ? `${BANNER_HEIGHT_PX}px` : 0, | |||
There was a problem hiding this comment.
add padding to bottom to account for banner height
| @@ -57,6 +62,7 @@ const Layout: React.FC<Props> = (props) => { | |||
| position: "relative", | |||
| height: "inherit", | |||
| overflowY: "auto", | |||
| paddingBottom: isBannerOpen ? `${BANNER_HEIGHT_PX}px` : 0, // add padding to bottom to account for banner height | |||
There was a problem hiding this comment.
Sidebar, adding padding to bottom to account for banner height so that banner doesn't cover scrollbar
| </div> | ||
| <div | ||
| style={{ | ||
| gridArea: "top / right-sidebar-start / bottom / right-sidebar-end", | ||
| position: "relative", | ||
| height: "inherit", | ||
| overflowY: "auto", | ||
| paddingBottom: isBannerOpen ? `${BANNER_HEIGHT_PX}px` : 0, // add padding to bottom to account for banner height |
There was a problem hiding this comment.
Sidebar, adding padding to bottom to account for banner height so that banner doesn't cover scrollbar
| @@ -73,18 +79,26 @@ const Layout: React.FC<Props> = (props) => { | |||
| }} | |||
| > | |||
| {graphComponent} | |||
| <Controls bottom={isBannerOpen ? BANNER_HEIGHT_PX : 0}> | |||
There was a problem hiding this comment.
Moving breadcrumbs here for finer control of styling so that the banner doesn't cover the breadcrumbs
| }} | ||
| > | ||
| {/* The below conditional is required because the right sidebar initializes as function for some reason...*/} | ||
| {!(rightSidebar instanceof Function) && rightSidebar} | ||
| </div> | ||
| <BottomBanner |
There was a problem hiding this comment.
Using the new Banner Component
There was a problem hiding this comment.
This file is almost exactly the same as https://github.com/chanzuckerberg/single-cell-data-portal/pull/4679/files#diff-49e8b71759ef76f78c483aa434ab80129c0bacb10bb315b0ed1ca2517196a091
But without the mobile view or footer view option
|
|
||
| interface Props { | ||
| includeSurveyLink: boolean; | ||
| setIsBannerOpen: React.Dispatch<React.SetStateAction<boolean>>; |
There was a problem hiding this comment.
This is for modifying the height/padding of the graph and sidebars to account for the banner height
| <> | ||
| <HeaderContainer> | ||
| <img alt="CELLxGENE Logo" src={cellxgeneLogoSvg} /> | ||
| <Button |
There was a problem hiding this comment.
we are on an older SDS version and ButtonIcon/IconButton does not have sdsStyle="xMark", so we are just using a button with "X" text. Slightly different than the close button on DP
| </> | ||
| )} | ||
| </div> | ||
| ) as unknown as string // For some reason setting the "text" prop overwrites the child, so we have to set the element in the text prop but it only takes a string as the type |
There was a problem hiding this comment.
This div is a child for StyledBanner in DP, but Explorer has an older SDS version so we have to include the element as a text prop, but the text prop only accepts type string
* feat: Newsletter Banner (#601) * feat: Newsletter Banner * fix: Allow Connection to Hubspot (#633) * fix: Allow Connection to Hubspot * moving to default-src * adding form-action * fix: Modify CSP to Allow Hubspot (#634) --------- Co-authored-by: Andrew Shin <109984998+ashin-czi@users.noreply.github.com>
* feat: Newsletter Banner (#601) * feat: Newsletter Banner * fix: Allow Connection to Hubspot (#633) * fix: Allow Connection to Hubspot * moving to default-src * adding form-action * fix: Modify CSP to Allow Hubspot (#634) --------- Co-authored-by: Andrew Shin <109984998+ashin-czi@users.noreply.github.com>
This reverts commit d99f066.
Reason for Change
Readability: @prathapsridharan
Functionality: @atarashansky
Changes
Testing steps
Notes for Reviewer
Implementation is almost exactly how Napari Hub integrates with Hubspot in order to prevent saving cookies. Their PR for integrating with Hubspot can be viewed here: https://github.com/chanzuckerberg/napari-hub/pull/661/files
explorer.newsletter.changes.mov
Note that the header/nav bar, breadcrumbs, graphs, and gene sets are placeholder values to mimic how actual Explorer would look from DP and to demonstrate scrolling of the left/right sidebars