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: move notification banner back to top and add banner type capabilities #359

Merged
merged 23 commits into from
Sep 29, 2020

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Sep 22, 2020

Problem

The current notification banners are not obvious enough, and may result in users missing the banner.
This PR updates the placement of the banners to the top, whilst changing the CSS to ensure no elements are blocked by banners if they exist.

Closes https://github.com/datagovsg/formsg-private/issues/28

Solution

Features:

  • add top aligned banner that does not block anything
  • update styling of app for sticky banner/navbar
  • add function to process banner message encoding to display in banner.client.component, along with documentation in DEPLOYMENT_SETUP.md

Improvements:

Bug Fixes:

Screenshots

AFTER:

SITE_BANNER_CONTENT=info:This is an info banner. You can also add links in the text like https://example.com. There is also a dismiss button to the right of the text.

Info banner example

SITE_BANNER_CONTENT=warn:This is a warning banner. You can also add links in the text like https://example.com

Warning banner example

SITE_BANNER_CONTENT=error:This is an error banner. You can also add links in the text like https://example.com

Error banner example

SITE_BANNER_CONTENT=hello:This is an invalid banner type, and the full text will be shown. The default banner type of `info` will used.

Invalid banner default example

Desktop GIF run-through:
recording (14)

Mobile GIF run-through:
https://streamable.com/20m5k9

Public form banner is now sticky:
recording (15)

@karrui
Copy link
Contributor Author

karrui commented Sep 22, 2020

Yes, I notice that the CSS is wonky on iOS safari............ will try to fix.

EDIT: fixed in dbb3be0

Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

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

minor suggestions!

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

minor comments/questions. tbh I'm not really sure what's going on in a lot of the CSS changes, I'm assuming they are refactors and you've tested them!

@karrui
Copy link
Contributor Author

karrui commented Sep 28, 2020

Nvm, ignore the CSS changes. Mobile scrolling is now screwed. Just iOS safari things, still can be scrolled since 100% height is still blocked by iPhone's notches....

@karrui karrui requested a review from mantariksh September 28, 2020 10:46
@karrui karrui merged commit 4b9a81f into develop Sep 29, 2020
@karrui karrui deleted the feat/banner-changes branch September 29, 2020 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants