Add status banner + fix templates 403 redirect loop #4684
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4684 +/- ##
========================================
Coverage 69.19% 69.19%
========================================
Files 145 145
Lines 5012 5012
Branches 768 768
========================================
Hits 3468 3468
Misses 1090 1090
Partials 454 454 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Love this Rory 🚀
How does the banner interact with normal notifications? Basically just wondering if it would be better to add a feature to notifications to show buttons (which I think is the only difference) and the just use the normal BBUI notifications module to show it. I think at the minute this will render above (depth wise) normal notifications will it?
Either that, or make the banner a full width header that actually shifts the page down or something, so it wouldn't affect notifications.
@aptkingston you're spot on it renders above normal notifications. Making this a normal notification would be a nice way to integrate them better, the only other usage of the banner is on the rest transformer here:
Which we could probably convert as well, although it might be strange to have a notification going outside of the normal usage and being embedded like that. I like the idea of making the banner display full width, which I think is probably the correct ux here as well - I'll take a look |
|
@aptkingston added the full width banner which helps a bit with the overlapping notifications:
after the banner is closed:
|
There was a problem hiding this comment.
Looks great @Rory-Powell! I much prefer the full width approach. Just left one suggestion about simplifying things and then it should be good to go 👍
| import { parse, stringify } from "qs" | ||
| import HelpIcon from "components/common/HelpIcon.svelte" | ||
|
|
||
| const queryHandler = { parse, stringify } | ||
| </script> | ||
|
|
||
| <div class="banner-container" /> | ||
| <BannerDisplay /> |
There was a problem hiding this comment.
This seems a bit strange to me - rendering a DOM node and then immediately after rendering a svelte portal which targets that DOM node. Why don't we just rename BannerDisplay to StatusBanner, remove the portal altogether and the .banner-container div, and just have the StatusBanner render a normal banner with status info in it's current location? That would achieve the same result but be a bit less complicated, and more explicit that it's intended to be used for status messages rather than a generic banner.
There was a problem hiding this comment.
Sounds like a good suggestion @aptkingston . I had a go at this but hit a roadblock around pointer events, the pointer isn't able to interact with with banner without the portal. Attached is the diff of what I tried, if you have any other suggestions of how to work around this I'd be up for trying them out. Gonna merge this one for now so that we can release the two fixes in the release
diff: status.txt
Description
Banner
Templates 403 loop
currentappcookie is present for an app in another tenant thebuilderandadminproperties are incorrectly removed from the current userScreenshots