feat: JSON Notifications #420
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…memoized selector Signed-off-by: TacosTonight <[email protected]>
…he app work with NotificationBanner in Layout Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Signed-off-by: TacosTonight <[email protected]>
Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…ate changes Signed-off-by: TacosTonight <[email protected]>
Signed-off-by: TacosTonight <[email protected]>
Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…nsitions Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Signed-off-by: TacosTonight <[email protected]>
Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| children: React.ReactNode; | ||
| } | ||
|
|
||
| export const Banner: FC<BannerProps> = ({ children }) => { |
There was a problem hiding this comment.
we can probably remove this file since it does nothing
There was a problem hiding this comment.
This component should be the presentational component. I suggest moving all the styling from NotificationBanner to this one. Banner should receive props like children and onClose.
| messages: [], | ||
| dismissedMessageIds: [], | ||
| statusMap: { | ||
| getNotificationMessages: { ...initialStatus }, |
There was a problem hiding this comment.
you can just do getNotificationMessages: initialStatus
| state.messages = messages.filter((message) => { | ||
| return message.active === true; | ||
| }); | ||
| state.statusMap.getNotificationMessages = {}; |
There was a problem hiding this comment.
this should be initialStatus now right?
There was a problem hiding this comment.
it does make sense to set it as initialStatus, since after the request is fulfilled, loading should be false and there shouldn't be an error.
I noticed in other reducers however that the .fulfilled is set to {}
There was a problem hiding this comment.
I prefer being more explicit and having initialStatus too, but as you have it also works now since we dont have expressions like if(getNotificationMessages.loading === false) .... But we need to be careful of that.
src/core/store/modules/notifications/notifications.selectors.ts
Outdated
Show resolved
Hide resolved
|
Instead of having the Banner span the either width of the page, I tried moving the NotificationBanner inside in containers/Layout. It looks ok to me, and makes it so the fixed positioning of the side nav bar css doesn't have to be changed. The scrolling and dismissing issues that I brought up in the PR description also gets solved. What does everyone else think? @xgambitox @FiboApe
|
Yeah, looks good to me :) I would just place the close button at the right of the banner. |
|
The thing having it there as an static thing, is that will cause ugly rerenders / resizing when you close it @TacosTonight @xgambitox |
|
So putting as position absolute is doable, but we'll have the same problem @TacosTonight was having before, where we need to push the navbars fixed position in the first render, and when the user scrolls down we need to revert that push, since the navbar will be pushed down but the notification banner not visible anymore. As for position fixed, the issue there is that the notificationBanner will be above information that the user might want to see while scrolling through the page. |
|
Another option for this, is to have our main wrapper have an overflow hidden on the main wrapper of the app, and then on the body have an overflow scroll. That will make it so the page always occupies the 100% of the screen, but the content can be scrolled. So like this: |
…ner to Banner. left notificationMessages.json filled out for testing, but will delete in final merge Signed-off-by: TacosTonight <[email protected]>
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| return <StyledBanner>{children}</StyledBanner>; | ||
| } | ||
|
|
||
| let messageSymbol: React.FunctionComponent<React.SVGProps<SVGSVGElement> & { title?: string | undefined }>; |
There was a problem hiding this comment.
you could probably use InfoIcon | WarningFilledIcon | WarningIcon | InfoIcon instead of redefining ti as React.FC
| import { Banner } from '@components/common'; | ||
|
|
||
| export const NotificationBanner: FC = () => { | ||
| const latestMessage = first(useAppSelector(NotificationsSelectors.selectActiveMessages)); |
There was a problem hiding this comment.
is there a reason we need a container component instead of just having this all on the banner component?
FoxTheSin
changed the title
|
Refactored style, fixes, layout, animation. |
Fixes #372
Definition of Done:
20Jan22 Update
Banner was converted from floating to occupying space in the DOM
Attempted to add more spacing to the banner, but some styling conflicts came up due to the side bar being in a fixed position. Requesting that the styling issue becomes a separate PR
Also attempted to hardcode the top position of the side bar, but scrolling down or dismissing messages leaves the side bar in an awkward position
