Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add survey banner to Explorer #1063

Merged
merged 18 commits into from
Aug 6, 2024
Merged

Conversation

rainandbare
Copy link
Contributor

@rainandbare rainandbare commented Aug 2, 2024

rdev --> https://valid-elk.dev-sc.dev.czi.team/d/super-cool-spatial.cxg/?spatial=true

Reason for Change

Changes

  • add BottomBanner with new Airtable Survey link to Explorer
  • uses the same local storage key as the data portal banner
  • ensured that Breadcrumbs are still visible with banner present

Bottom Banner Visible and Clickable

Screenshot 2024-08-02 at 12 43 18 PM

Testing steps

  • Check Explorer and see bottom banner
  • Click survey link to take you to correct airtable survey
  • Click x button to dismiss banner - it will not return for 30 days after you do this (except in incognito browser)
  • Check bottom banner on mobile screens as will as large.
  • Check that graph resizes correctly after you close the banner

Checklist 🛎️

  • Add product, design, and eng as reviewers for rdev review
  • For UI changes, add screenshots/videos, so the reviewers know what you expect them to see
  • For UI changes, add e2e tests to prevent regressions
  • For UI changes, verify impacted analytics events still work

Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

LGTM! Just one discussion regarding the module export strategy 💡 Thank you!!

@@ -0,0 +1,12 @@
import BottomBanner from "./BottomBanner";
Copy link
Contributor

Choose a reason for hiding this comment

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

discussion: should we put BottomBanner.tsx code in the index.tsx file here as a named export to avoid double exports 😄 ?

e.g.,

export const BottomBanner = connect(
  mapStateToProps,
  mapDispatchToProps
)(_BottomBanner);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - lets do that for now! I wanted to suggest a new structure for the index files and folders but that is a bigger discussion and requires a little more time! Thanks for the review!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good thank you so much 🚀 ! Yeah would love to learn more about it once the launch is done 👍

@rainandbare rainandbare added stack and removed stack labels Aug 6, 2024
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

LGTM!!

@rainandbare rainandbare added stack and removed stack labels Aug 6, 2024
@rainandbare rainandbare merged commit a875df6 into main Aug 6, 2024
22 checks passed
@rainandbare rainandbare deleted the rainandbare/7314-add-banner branch August 6, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants