Show a banner based on some url param #5620

erikdstock · 2020-05-20T22:13:05Z

Related 🔒 Jira Ticket
Similar Eigen PR: artsy/eigen#3325

Initial working solution to a problem: I want to show a banner when the user passes email confirmation. This does it by catching the param very early in express, adding it to sd and then passing it into our NavBar component. It's a little bit over-engineered for what it is doing right now, a result of my trying to understand existing practices and where things were coming from.

Questions here:

Any reason to NOT show it on potentially every page?
The banner lays on top of other content, outside the document flow.
Related eigen pr: [AUCT-1011] - Add confirmation alert for email verification in homeVC eigen#3325. We will want to at least make sure we are using the same query parameter. Currently we are not.
I'm not sure if I added bannerCode, my new banner-related NavBar prop to too many places (eg a NavBar in a jade template).

src/desktop/components/react/stitch_components/NavBar.tsx

src/desktop/components/main_layout/header/templates/index.jade

erikdstock · 2020-05-21T15:58:22Z

src/mobile/components/layout/templates/main.jade

@@ -7,11 +7,11 @@ block body
      block banner
        mixin login-signup("header")
      header
-        != stitch && stitch.components.NavBar({ user: sd.CURRENT_USER, notificationCount: sd.NOTIFICATION_COUNT })
+        != stitch && stitch.components.NavBar({ user: sd.CURRENT_USER, flashMessage: sd.flashMessage, notificationCount: sd.NOTIFICATION_COUNT })


Wondering what the difference is between this and index.jade, and why we have 2 navbars in this file...

I believe I am responsible for the double-header. 😅 (#5261)

I think what you have is fine, but note that the sticky-header behavior on mobile web for non-reaction pages is a little weird. I would verify that this looks fine on the home page (logged in and logged out) and on the artist page (logged in/out) to be sure that nothing's messed up!

src/lib/setup.js

damassi · 2020-05-22T20:47:02Z

@erikdstock - if y'all can hold off on merging this. Very close with the other PR and then y'all can rebase.

We're going to do a release of some analytics updates but it would be best if this wasn't in that batch.

dleve123 · 2020-05-22T20:58:26Z

Re #5620 (comment)

That's fine! We're working on adding some tests for this behavior now. Can you ping us when the other PR is off and away?

ArtsyOpenSource · 2020-05-22T21:32:00Z

	Fails
🚫	Danger failed to run `dangerfile.ts`.

Error RangeError

Maximum call stack size exceeded
RangeError: Maximum call stack size exceeded
    at RegExp.[Symbol.replace] (<anonymous>)
    at String.replace (<anonymous>)
    at standardizeName (/home/circleci/project/node_modules/@babel/core/lib/config/files/plugins.js:94:15)
    at resolveStandardizedName (/home/circleci/project/node_modules/@babel/core/lib/config/files/plugins.js:98:28)
    at resolvePlugin (/home/circleci/project/node_modules/@babel/core/lib/config/files/plugins.js:54:10)
    at loadPlugin (/home/circleci/project/node_modules/@babel/core/lib/config/files/plugins.js:62:20)
    at createDescriptor (/home/circleci/project/node_modules/@babel/core/lib/config/config-descriptors.js:154:9)
    at /home/circleci/project/node_modules/@babel/core/lib/config/config-descriptors.js:109:50
    at Array.map (<anonymous>)
    at createDescriptors (/home/circleci/project/node_modules/@babel/core/lib/config/config-descriptors.js:109:29)

Dangerfile

-----------------^

Generated by 🚫 dangerJS against 89be270

codecov · 2020-05-22T21:45:54Z

Codecov Report

Merging #5620 into master will decrease coverage by 0.0%.
The diff coverage is 0.0%.

@@           Coverage Diff            @@
##           master   #5620     +/-   ##
========================================
- Coverage    79.3%   79.3%   -0.1%     
========================================
  Files        1250    1250             
  Lines       34034   34035      +1     
  Branches     2072    2073      +1     
========================================
  Hits        27021   27021             
- Misses       5999    6000      +1     
  Partials     1014    1014

damassi · 2020-05-22T21:59:15Z

@erikdstock - Regarding approach, one thought is: rather than passing this data all through the system in order to SSR render, we simply look at the url in a useEffect and if the flag is there, show the bar. Even better, this can be moved into a hook and invoked in the NavBar component:

const flashMessage = useFlashMessage()
...
<Box>
  {flashMessage?.message && <FlashMessage message={flashMessage.message} />}
</Box>

This can happen all on the client. As that url bar query param state will never be persisted across urls we don't need to worry about it flashing in when a user clicks something and a new page re-renders.

Thoughts?

erikdstock · 2020-05-22T22:59:37Z

@damassi Re: #5620 (comment)

Talking with @dleve123 much prefer this idea and spiked on it for a bit but weren't able to get it completely working. We'd like to merge this if tests pass and then iterate on it next week when we return for the more complicated next steps on the FlashBanner component, at which point (once working) we will undo all changes related to sd.FLASH_BANNER.

damassi · 2020-05-22T23:09:07Z

It might be worth reaching out to #dev-help if y'all are running into trouble with the client-side approach (versus rewinding everything)? A quick glance at https://github.com/artsy/force/pull/5620/files#diff-3df7ffd25fef9729a61746506a33251bR34 and i think this could be done with just a couple lines of code.

Co-authored-by: Erik Stockmeier <erik.d.stock@gmail.com>

Co-authored-by: Daniel Levenson <dleve123@gmail.com>

erikdstock · 2020-05-26T16:02:03Z

@damassi re #5620 (comment)

@dleve123 and I were able to get it working this morning, much better- thanks for the tip!

src/v2/Components/FlashBanner.tsx

damassi · 2020-05-26T19:42:56Z

src/v2/Components/FlashBanner.tsx

+    }
+  }, [])
+
+  if (!messageCode) return null


Maybe move this above the effect to show that we're exiting early and to avoid registering the effect inside of react's execution loop?

I'm not sure I follow you here.

Meaning, if (!messageCode) return null can move up above the React.useEffect call. Since messageCode is null, and we know that via props, we can immediately check for it right at the top of the component and exit early.

src/v2/Components/FlashBanner.tsx

erikdstock requested review from damassi and mzikherman May 20, 2020 22:13

erikdstock self-assigned this May 20, 2020

erikdstock mentioned this pull request May 20, 2020

email confirmation message banner artsy/reaction#3576

Merged

damassi reviewed May 20, 2020

View reviewed changes

src/desktop/components/react/stitch_components/NavBar.tsx Outdated Show resolved Hide resolved

xtina-starr reviewed May 20, 2020

View reviewed changes

src/desktop/components/react/stitch_components/NavBar.tsx Outdated Show resolved Hide resolved

erikdstock commented May 21, 2020

View reviewed changes

dleve123 force-pushed the auct1001-show-email-confirmation-banner branch from 403f450 to 1c121d9 Compare May 22, 2020 20:23

dleve123 marked this pull request as ready for review May 22, 2020 20:23

dleve123 changed the title ~~WIP: Show a banner based on some url param~~ Show a banner based on some url param May 22, 2020

erikdstock and others added 7 commits May 26, 2020 11:09

WIP: Show a banner based on some url param

e42b1bc

rename banner_code query param to flashMessage`

4eea8e2

Smoke test that flash messages behave appropriately

65cfce3

Co-authored-by: Erik Stockmeier <erik.d.stock@gmail.com>

Refactor: Use existing locals middleware to process flashMessage

b4c8d31

Co-authored-by: Erik Stockmeier <erik.d.stock@gmail.com>

wip

52ba5c6

use hooks to check location instead of sd+props

2106f12

Move flash message displaying logic into effect rather than sd

9d421c4

Co-authored-by: Daniel Levenson <dleve123@gmail.com>

erikdstock force-pushed the auct1001-show-email-confirmation-banner branch from 83b7e7a to 9d421c4 Compare May 26, 2020 16:00

erikdstock added 2 commits May 26, 2020 15:35

fix timing out acceptance tests

b374cd7

gitignore node diagnostic reports

113d876

damassi reviewed May 26, 2020

View reviewed changes

src/v2/Components/FlashBanner.tsx Outdated Show resolved Hide resolved

damassi reviewed May 26, 2020

View reviewed changes

src/v2/Components/FlashBanner.tsx Outdated Show resolved Hide resolved

damassi reviewed May 26, 2020

View reviewed changes

src/v2/Components/FlashBanner.tsx Show resolved Hide resolved

break FlashBanner into multiple files

b4b15bc

erikdstock force-pushed the auct1001-show-email-confirmation-banner branch from 96ca972 to b4b15bc Compare May 26, 2020 20:25

erikdstock and others added 3 commits May 26, 2020 17:02

cleanup

1aad4ae

simplify and add more tests

488731d

Add cypress tests for flash message

89be270

erikdstock merged commit 5e4e705 into artsy:master May 27, 2020

artsy-peril bot mentioned this pull request May 27, 2020

Deploy #5652

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a banner based on some url param #5620

Show a banner based on some url param #5620

erikdstock commented May 20, 2020 •

edited

Loading

erikdstock May 21, 2020

sweir27 May 22, 2020

damassi commented May 22, 2020

dleve123 commented May 22, 2020

ArtsyOpenSource commented May 22, 2020 •

edited

Loading

codecov bot commented May 22, 2020

damassi commented May 22, 2020 •

edited

Loading

erikdstock commented May 22, 2020

damassi commented May 22, 2020 •

edited

Loading

erikdstock commented May 26, 2020

damassi May 26, 2020

erikdstock May 26, 2020

damassi May 26, 2020

Show a banner based on some url param #5620

Show a banner based on some url param #5620

Conversation

erikdstock commented May 20, 2020 • edited Loading

erikdstock May 21, 2020

Choose a reason for hiding this comment

sweir27 May 22, 2020

Choose a reason for hiding this comment

damassi commented May 22, 2020

dleve123 commented May 22, 2020

ArtsyOpenSource commented May 22, 2020 • edited Loading

Error RangeError

Dangerfile

codecov bot commented May 22, 2020

Codecov Report

damassi commented May 22, 2020 • edited Loading

erikdstock commented May 22, 2020

damassi commented May 22, 2020 • edited Loading

erikdstock commented May 26, 2020

damassi May 26, 2020

Choose a reason for hiding this comment

erikdstock May 26, 2020

Choose a reason for hiding this comment

damassi May 26, 2020

Choose a reason for hiding this comment

erikdstock commented May 20, 2020 •

edited

Loading

ArtsyOpenSource commented May 22, 2020 •

edited

Loading

damassi commented May 22, 2020 •

edited

Loading

damassi commented May 22, 2020 •

edited

Loading