Skip to content

Turn off Notification Center from Preferences Panel.#13190

Merged
QilongTang merged 7 commits intomasterfrom
NotificationCenter
Aug 11, 2022
Merged

Turn off Notification Center from Preferences Panel.#13190
QilongTang merged 7 commits intomasterfrom
NotificationCenter

Conversation

@QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Aug 9, 2022 •
edited
Loading

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Per https://jira.autodesk.com/browse/DYN-5177, New setting entry was added in Dynamo setting: EnableNotificationCenter

  • Notification Center now can be disabled in the Preferences panel
  • The Notification bell has been moved to the right side of the export menu
  • The notification bell got a new look
  • Updated unit tests also to include serializing and deserializing the new setting

image

image

image

NotificationBellDisable

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Users can turn off Notification Center from the Preferences Panel.

Reviewers

@DynamoDS/dynamo

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@QilongTang QilongTang added this to the 2.16.0 milestone Aug 9, 2022
@Jingyi-Wen
Copy link

Jingyi-Wen commented Aug 9, 2022

Hi @QilongTang , just a suggestion - can we change the text for the toggle to "Receive notification"? Since the header already says Notification Center, it seems a bit redundant.

@QilongTang
Copy link
Contributor Author

QilongTang commented Aug 9, 2022

eems

Will do

@QilongTang QilongTang marked this pull request as ready for review August 9, 2022 20:56
zeusongit
zeusongit reviewed Aug 9, 2022
View reviewed changes
zeusongit
zeusongit reviewed Aug 9, 2022
View reviewed changes
src/DynamoCoreWpf/ViewModels/Menu/PreferencesViewModel.cs
}
set
{
isolateSelectedGeometry = value;
Copy link
Contributor

@zeusongit zeusongit Aug 9, 2022

Choose a reason for hiding this comment

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

why are we removing these setters here? Are these not used anymore?

Copy link
Contributor Author

@QilongTang QilongTang Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@zeusongit Only removing the private value setter since these private values are not used. Suggested by VS

RobertGlobant20
RobertGlobant20 approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a comment

Choose a reason for hiding this comment

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

LGTM

@QilongTang
Copy link
Contributor Author

QilongTang commented Aug 10, 2022

@RobertGlobant20 @filipeotero Any idea how to change the hover-over background of the notification bell?

@RobertGlobant20
Copy link
Contributor

RobertGlobant20 commented Aug 10, 2022

@RobertGlobant20 @filipeotero Any idea how to change the hover-over background of the notification bell?

@QilongTang I think you need to define a Style in DynamoModern.xaml file (or in the same xaml file, ShortcutToolbar.xaml), at the end the FontAwesome behaves in a similar way than any wpf control.
image
and then apply the colors for foreground, background using ControlTemplate.Triggers, something like this:
image
Let me know if it works. Thanks

@QilongTang
Copy link
Contributor Author

QilongTang commented Aug 10, 2022

@RobertGlobant20 This is what I am currently adding, but it keep crashing Dynamo while build is fine:

    <Style x:Key="NotificationBellButtonStyle" TargetType="{x:Type fa:FontAwesome}">
        <Setter Property="Height" Value="Auto" />
        <Setter Property="Template">
            <Setter.Value>
                <ControlTemplate TargetType="{x:Type MenuItem}">
                    <ControlTemplate.Triggers>
                        <Trigger Property="IsMouseOver" Value="True">
                            <Setter TargetName="ContentPresenter" Property="Background" Value="{StaticResource NodeContextMenuBackgroundHighlight}" />
                        </Trigger>
                        <Trigger Property="IsMouseOver" Value="False">
                            <Setter TargetName="ContentPresenter" Property="Background" Value="{StaticResource NodeContextMenuBackground}" />
                        </Trigger>
                    </ControlTemplate.Triggers>
                </ControlTemplate>
            </Setter.Value>
        </Setter>
    </Style>

                                    <fa:FontAwesome Margin="0,0,25,0"  
                                                    DockPanel.Dock="Top" 
                                                    Icon="{Binding Path=DataContext.PreferenceSettings.EnableNotificationCenter, RelativeSource={RelativeSource AncestorType={x:Type Window}}, 
                                                         Converter={StaticResource BoolToFAIconNameConverter}}"
                                                    Style="{StaticResource NotificationBellButtonStyle}"
                                                    HorizontalAlignment="Left"
                                                    VerticalAlignment="Center"
                                                    Foreground="{StaticResource PrimaryCharcoal300Brush}"
                                                    FontSize="18px"/>

@RobertGlobant20
Copy link
Contributor

RobertGlobant20 commented Aug 11, 2022 •
edited
Loading

@QilongTang probably the crash is happening due that you are using ContentPresenter, I tried to use your style but it was not compiling because of the word "Template".
But the next Style worked for me (let me know if it helps):
image
image
notification_bell_background

@QilongTang
Copy link
Contributor Author

QilongTang commented Aug 11, 2022 •
edited
Loading

@RobertGlobant20 Thanks! If I just apply the style on fontawesome, it works while only on the bell itself. Since the whole Notification Bell + message count is a button, so I had to apply the style to the button which does not work :( I believe that is what I tried before but not sure if I implemented anything wrong.

    <Style x:Key="NotificationBellStyle" TargetType="{x:Type Button}">
        <Style.Triggers>
            <Trigger Property="IsMouseOver" Value="True">
                <Setter Property="Background" Value="White" />
            </Trigger>
        </Style.Triggers>
    </Style>

@QilongTang QilongTang merged commit 8d818b7 into master Aug 11, 2022
@QilongTang QilongTang deleted the NotificationCenter branch August 11, 2022 14:38
@QilongTang
Copy link
Contributor Author

QilongTang commented Aug 11, 2022

@RobertGlobant20 Discussed with team, will merge first and follow up with you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeusongit zeusongit zeusongit left review comments

@RobertGlobant20 RobertGlobant20 RobertGlobant20 approved these changes

@reddyashish reddyashish Awaiting requested review from reddyashish

@filipeotero filipeotero Awaiting requested review from filipeotero

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.16.0

Development

Successfully merging this pull request may close these issues.

4 participants

@QilongTang @Jingyi-Wen @RobertGlobant20 @zeusongit