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

Fix padding when notification is visible #1264

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

axpoems
Copy link
Contributor

@axpoems axpoems commented Oct 13, 2023

Before, the notification padding was not being updated properly while navigating through the different pages while the notification was visible. Proper padding handling for the special case of the tabView is achieved by changing the responsibility of handling this to contentView instead of the notification panel.

After
fix-notification-after

Before
fix-notification-before

Copy link
Contributor

@HenrikJannsen HenrikJannsen left a comment

Choose a reason for hiding this comment

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

Here is my suggestion to avoid singletons: https://github.com/HenrikJannsen/bisq2/tree/fix-notification-padding_suggested_changes Feel free to pick out from it what you like. I don't care about authorship, I just created a branch as it was a more complex change and I needed to try out myself to see if that approach is feasible.

@axpoems
Copy link
Contributor Author

axpoems commented Oct 13, 2023

Thanks for the suggestion. I have reviewed your commits and code and like your solution.
I had also considered both options: passing the NotificationPaneController or using the NotificationService to achieve this.
Getting inspiration from what you mention about persisting the close state and looking at your code, I have picked up the branch I was working prior to this and improved it - so this is my new proposal: https://github.com/axpoems/bisq2/tree/bugfix-notification-panel

@HenrikJannsen
Copy link
Contributor

HenrikJannsen commented Oct 14, 2023

https://github.com/axpoems/bisq2/tree/bugfix-notification-panel looks good to me.

I am not so 100% sure if its best to let the TabController/View know so much about the environment. To let the parent container view handle it would sound more correct to me. But I am fine with that as well. We don't need to over-think it either....

I am also not so sure if the previous version with having a gap between tab line and scroll area was better. But I leave it up to you. Not too strong onion here. Both version do not feel perfect. Maybe we come up later with an alternative solution...

Maybe add a comment to isNotificationDismissed why it is not persisted (that we intentionally prefer to let the notification popup again after restart).

@HenrikJannsen
Copy link
Contributor

After another thought I think doing the handling inside tabview/controller does not work as we use tabviews inside popups as well. I think it should be handled in the container (contentview)

@axpoems axpoems force-pushed the fix-notification-padding branch 2 times, most recently from 8456b57 to 74ee3c1 Compare October 16, 2023 06:39
@axpoems
Copy link
Contributor Author

axpoems commented Oct 16, 2023

After another thought I think doing the handling inside tabview/controller does not work as we use tabviews inside popups as well. I think it should be handled in the container (contentview)

I agree. The way the code is setup it makes sense that notification handling is done from parent of tabview. I have updated the code with your proposal.

I am also not so sure if the previous version with having a gap between tab line and scroll area was better. But I leave it up to you. Not too strong onion here. Both version do not feel perfect. Maybe we come up later with an alternative solution...

Agree as well that non of these solutions look perfect. I have reverted it to previous version. Will look more into this. I think the problem here might be that background color is the same for tabview and content.

@axpoems axpoems force-pushed the fix-notification-padding branch 6 times, most recently from 063af42 to 7f25b4c Compare October 22, 2023 18:59
axpoems and others added 5 commits October 22, 2023 21:02
Before, the notification padding was not being updated properly while
navigating through the different pages while the notification was visible.
Proper padding handling for the special case of the tabView is achieved
by changing the responsibility of handling this to contentView instead of the
notification panel.

Co-authored-by: HenrikJannsen <boilingfrog@gmx.com>
Move handling of notification panel visibility to BisqEasyNotificationsService.
Add handling of paddings from notification panel visibility to DashboardView.
Use light tab line style as default. Add left/right padding to tab line.
Improve NetworkInfoView.
@axpoems axpoems force-pushed the fix-notification-padding branch from 7f25b4c to 9a0dfd7 Compare October 22, 2023 19:03
Copy link
Contributor

@HenrikJannsen HenrikJannsen left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

@alvasw alvasw left a comment

Choose a reason for hiding this comment

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

utACK

@alvasw alvasw merged commit 0c6d1ab into bisq-network:main Oct 23, 2023
12 checks passed
@axpoems axpoems deleted the fix-notification-padding branch December 30, 2023 07: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