feat: security banner concept #9780

ackernaut · 2024-05-23T20:25:57Z

Concept

Adds a banner with color and message options. Fixes #9778

Demo

Loom 1 of 3 — Proof of Concept
Loom 2 of 3 — Fixed Banner in All App Views
Loom 3 of 3 — Layout Accounts for Banner

Todo

Does the banner cause any overflow issues in the dashboard viewports when views are overflowing with content?
Add .env vars to provide message and style (change vars to generic banner naming convention)
Make sure banner shows in all views (e.g. move to Action.ts)
Tested: remote reflection drag and drop has accurate coordinates

Test

The banner is fixed at the top of all app views — it doesn’t scroll away
The banner is set using .env variables including options for text, text color, and background colors
Remote reflection visual feedback (other users are dragging a reflection and you see it) has accurate coordinates (use a second browser and user to test this)
Check all views when banner is disabled for desktop
Check all views when banner is enabled for desktop
Check all views when banner is disabled for mobile
Check all views when banner is enabled for mobile
Views to check:
- auth page
- /invitation-link/12345678 view
- 404
- dash timeline, meetings, tasks
- dash team, team tasks
- dash team drawers (manage team, agenda)
- dash team invite dialog
- dash meeting summary
- org admin views
- org admin billing drawer
- activity library
- activity library details
- meeting nav
- meeting layout
- poker discussion drawer
- stand-up response thread drawer
- stand-up work drawer
- recurring retro choose template drawer
- retro demo

packages/client/components/Dashboard.tsx

mattkrick · 2024-05-29T21:41:13Z

For QA, we may have a spot or 2 in the app that calculates coordinates assuming a banner doesn't exist, e.g. remotely dragged reflections & oauth popup windows. Chances are any calculation that adds a hardcoded number to innerHeight from window could be vulnerable

ackernaut · 2024-05-30T15:31:42Z

For QA, we may have a spot or 2 in the app that calculates coordinates assuming a banner doesn't exist, e.g. remotely dragged reflections & oauth popup windows. Chances are any calculation that adds a hardcoded number to innerHeight from window could be vulnerable

Good to know @mattkrick — if I set a height to the banner, can we add or subtract that amount to the innerHeight (have to see how it works)?

mattkrick · 2024-05-30T16:24:39Z

I think it'll just be a case-by-case basis, if thing look good with the banner, we're good!

mattkrick

it looks good! the only downfall is that this adds quite a bit of complexity to our whole app. before merging, can we make sure that there's no simpler way to do this? I think we could probably keep the logic constrained to the parent <Action/> component

scripts/webpack/dev.client.config.js

mattkrick · 2024-05-31T00:27:56Z

packages/client/components/ActivityLibrary/ActivityDetails/ActivityDetails.tsx

  return (
    <div className='flex h-full w-full flex-col bg-white'>
      <div className='flex grow'>
-        <div className='mt-4 w-full grow'>
+        <div className={clsx('mt-4 w-full grow', isGlobalBannerEnabled && 'pt-6')}>


-1 as we build out our app, it'd be pretty easy to forget to include this logic! Instead of making sure this is included in every child component, could we just keep it in <Action/>? We can wrap the <Switch/> with <div className={isGlobalBannerEnabled ? 'pt-6' : ''}>

Maybe, I can try this and see if layout breaks.

The problem is we have some components that use 100vh so we’d probably at least have to touch those. I can see about reducing the scope to <Action /> and any components using 100vh

ackernaut · 2024-05-31T16:50:21Z

devTemplate.html

@@ -1,4 +1,4 @@
-<!DOCTYPE html>
+<!doctype html>


This was due to auto-formatting

ackernaut · 2024-05-31T16:54:51Z

@mattkrick I added a wrapper around the <Switch /> in <Action /> and one around both the Banner and the inner wrapper to control sizing more consistently from a single place. I still had to apply some conditional styling to side sheets that use 100vh — still haven’t found a good way to do that since the swipeable/transition parents are generated by the library.

<div>
  <Banner />
  <div>
    <Switch/>
  </div>
</div>

ackernaut

Ready for additional review and testing!

dbumblis-parabol · 2024-06-03T19:35:59Z

I noticed if the env var does not exist then the banner still appears with no text. I think the banner should not appear unless the env var is explicitly set

dbumblis-parabol · 2024-06-03T20:45:32Z

Testing logic changes and smoke tested the views. LGTM! @mattkrick if things look ok to you let's ship it

mattkrick · 2024-06-03T20:53:15Z

packages/client/components/Action/Action.tsx

-                path={`/forgot-password`}
-                render={(p) => <AuthenticationPage {...p} page={'forgot-password'} />}
-              />
+          <div className='flex h-full max-h-screen flex-col'>


in testing, i deleted this div & everything still worked OK, is there a use case where this is required?

This one allows for better control of the main height by wrapping both the banner and the main view. I think there may be one other way I could try. Let me test.

mattkrick · 2024-06-03T20:54:17Z

packages/client/components/Action/Action.tsx

+            {/* Set up a container for the global banner and the main app area */}
+            <div
+              className={clsx(
+                'h-100 flex flex-1 flex-col overflow-hidden',


can we get rid of everything here except the pt-6? having overflow-hidden as a parent could make things difficult because that means no component will be able to use the viewport scroll

I think it was hard to control height of inner components on all the main views. In other words, I can do it on this one view and then tweak a few things here in there, or I have to go to most of the main views and adjust heights and what not. I’m not sure how much that would have to be conditional based on the GLOBAL_BANNER_ENABLED which I was trying to use less. So much of the app has to control inner scrolling. Maybe only mobile is relying on true viewport scrolling?

I’ve got something I want to try but I think we should consider moving forward and making an additional set of improvements in a later PR.

ackernaut · 2024-06-04T17:05:15Z

Trying to remove those new outermost containers, I run into the same problems I fixed with them, go figure. I’m not sure how to get containers to behave. We have many, many containers where we are controlling the scrolling, and we have many views that layout to fit the viewport with inner scrolling. Introducing a global banner affects all views. Not sure if or how long it will take to try to find a way to achieve the tight views without odd scrolling etc due to the banner. We could ship this as it is timely, and then maybe clean up the container later. I’ll see if I can get somewhere with another approach that plays nice with all views, but until then I think it’s shippable. Good over perfect.

packages/client/components/Action/Action.tsx

packages/client/components/GlobalBanner.tsx

ackernaut · 2024-06-05T21:43:17Z

@mattkrick thanks for reviewing, do I merge now or do you or @Dschoordsch have to merge?

security banner concept

cba3ba4

github-actions bot added the size/s label May 23, 2024

ackernaut added 4 commits May 23, 2024 15:27

color reference

85f9a61

sec banner conditional

921834d

try text and style banner vars

7b6fbc6

props override default levels

cff759a

ackernaut changed the title ~~Security banner concept~~ Feat: security banner concept May 23, 2024

ackernaut changed the title ~~Feat: security banner concept~~ feat: security banner concept May 23, 2024

ackernaut added 2 commits May 23, 2024 16:42

Remove defaults

7a1c3d6

banner style reference

df663d8

Dschoordsch reviewed May 27, 2024

View reviewed changes

packages/client/components/Dashboard.tsx Outdated Show resolved Hide resolved

packages/client/components/Dashboard.tsx Outdated Show resolved Hide resolved

ackernaut added 2 commits May 30, 2024 15:15

Sets up global banner

1131f54

Applies padding to account for fixed global banner

3aa9659

github-actions bot added size/m and removed size/s labels May 30, 2024

mattkrick requested changes May 31, 2024

View reviewed changes

ackernaut added 4 commits May 30, 2024 20:34

Fix env var to enable banner

e8c1360

moves more layout to Action container

ae4485c

clean up

5c6b897

fixes wrapper around switch instead of the other way

20974f6

ackernaut commented May 31, 2024

View reviewed changes

devTemplate.html Outdated

@@ -1,4 +1,4 @@

<!DOCTYPE html>

<!doctype html>

Copy link

Member Author

ackernaut May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was due to auto-formatting

ackernaut added 2 commits May 31, 2024 12:15

fix overflow due to new wrappers

8960ccd

Add more overflow to inner containers

59ed346

ackernaut commented May 31, 2024

View reviewed changes

ackernaut added 2 commits June 3, 2024 10:14

fix stand-up thread and work drawer

d910f73

nit fix auth page

15547f6

nit fix meeting sidebar again

595dbe3

ackernaut added 2 commits June 3, 2024 15:10

update the env boolean var

780111a

updates example env

cdfa339

mattkrick reviewed Jun 3, 2024

View reviewed changes

ackernaut added 2 commits June 4, 2024 12:38

try new Action container layout

ae139c8

comments

eb12828

ackernaut commented Jun 4, 2024

View reviewed changes

packages/client/components/Action/Action.tsx Show resolved Hide resolved

ackernaut commented Jun 4, 2024

View reviewed changes

packages/client/components/Action/Action.tsx Show resolved Hide resolved

ackernaut commented Jun 4, 2024

View reviewed changes

packages/client/components/GlobalBanner.tsx Show resolved Hide resolved

mattkrick approved these changes Jun 5, 2024

View reviewed changes

ackernaut self-assigned this Jun 5, 2024

undo auto-formatting changes to templates

abee000

ackernaut merged commit b683dc8 into master Jun 6, 2024
6 checks passed

ackernaut deleted the feat/sec-banner branch June 6, 2024 15:14

parabol-release-bot bot mentioned this pull request Jun 6, 2024

chore(release): release v7.36.0 #9832

Merged

github-actions bot mentioned this pull request Jun 10, 2024

chore(release): Test v7.36.0 #9837

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: security banner concept #9780

feat: security banner concept #9780

ackernaut commented May 23, 2024 •

edited

Loading

mattkrick commented May 29, 2024

ackernaut commented May 30, 2024

mattkrick commented May 30, 2024

mattkrick left a comment

mattkrick May 31, 2024

ackernaut May 31, 2024

ackernaut May 31, 2024

ackernaut May 31, 2024

ackernaut commented May 31, 2024

ackernaut left a comment

dbumblis-parabol commented Jun 3, 2024 •

edited

Loading

dbumblis-parabol commented Jun 3, 2024

mattkrick Jun 3, 2024

ackernaut Jun 4, 2024

mattkrick Jun 3, 2024

ackernaut Jun 4, 2024 •

edited

Loading

ackernaut Jun 4, 2024

ackernaut commented Jun 4, 2024

ackernaut commented Jun 5, 2024

feat: security banner concept #9780

feat: security banner concept #9780

Conversation

ackernaut commented May 23, 2024 • edited Loading

Concept

Demo

Todo

Test

mattkrick commented May 29, 2024

ackernaut commented May 30, 2024

mattkrick commented May 30, 2024

mattkrick 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

ackernaut commented May 31, 2024

ackernaut left a comment

Choose a reason for hiding this comment

dbumblis-parabol commented Jun 3, 2024 • edited Loading

dbumblis-parabol commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackernaut Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackernaut commented Jun 4, 2024

ackernaut commented Jun 5, 2024

ackernaut commented May 23, 2024 •

edited

Loading

dbumblis-parabol commented Jun 3, 2024 •

edited

Loading

ackernaut Jun 4, 2024 •

edited

Loading