feat: Added a Notification head bar to present current notifications number #1322
Conversation
…hevron icon as notifications expand/shrink control (no toggle logic yet) Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
…h current notifications number Signed-off-by: Jian Wang <[email protected]>
…snapshot Signed-off-by: Jian Wang <[email protected]>
…or due to snapshot recording in different local env Signed-off-by: Jian Wang <[email protected]>
| @@ -37,12 +37,13 @@ const Home: React.FC<HomeProps> = () => { | |||
| const { ColorPallet } = useTheme() | |||
| const [store, dispatch] = useStore() | |||
| const { start } = useTour() | |||
| const [showNotifications] = useState(true) | |||
There was a problem hiding this comment.
This state variable isn't changed and is used in more than one component. Would it be better adding it as a config option? (TOKENS.CONFIG)
There was a problem hiding this comment.
Otherwise I don't really see the purpose, as it's not overideable and isn't ever changed. If that's fine then maybe we can just remove it entirely?
There was a problem hiding this comment.
Removed the chevron and showNotifications state, will add them back in story implements them
| ListHeaderComponent={() => { | ||
| const homeHeaderViewProps = { | ||
| notifications: notifications, | ||
| showNotifications: showNotifications, | ||
| } | ||
| return ( | ||
| <HomeHeaderView {...homeHeaderViewProps} /> | ||
| ) | ||
| }} |
There was a problem hiding this comment.
| ListHeaderComponent={() => { | |
| const homeHeaderViewProps = { | |
| notifications: notifications, | |
| showNotifications: showNotifications, | |
| } | |
| return ( | |
| <HomeHeaderView {...homeHeaderViewProps} /> | |
| ) | |
| }} | |
| ListHeaderComponent={() => <HomeHeaderView notifications={notifications} /> } |
If the showNotifications gets moved into config, this might be a bit cleaner
| paddingBottom: !showNotifications ? 0 : index === notifications.length - 1 ? 20 : 10, | ||
| backgroundColor: ColorPallet.brand.secondaryBackground, | ||
| }} | ||
| > | ||
| {DisplayListItemType(item)} | ||
| { showNotifications && DisplayListItemType(item) } |
There was a problem hiding this comment.
showNotifications can be removed unless it becomes changeable / configurable. If it does become configurable, then it should simply not render these list items at all. No empty Views with padding and background needed
There was a problem hiding this comment.
Removed the chevron icons and the showNotification state, will add them back in next story with the implementations
Signed-off-by: Jian Wang <[email protected]>
|
|
Can the head bar be toggleable/Is it toggleable on/off? |
No interaction control to toggle the notification numbers head bar, but it will disappear after all notifications are dismissed. So we can say it is toggled by current notifications list. Thanks @knguyenBC |
@jian4on BC Wallet team would like to choose not to include this feature into our wallet as it will cause some UI issues. If the component can be built so that we can choose to have this enabled off. |
Summary of Changes
Improved the HomeHeaderView component by adding a Notifications head bar, and maintained the test suits for new changes
Related Issues
N/A
Pull Request Checklist
Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.
Signed-off-byline (we use the DCO GitHub app to enforce this);If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
Pro Tip 🤓
PR template adapted from the Python attrs project.