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

Add "new" labels to navigation #36939

Merged
merged 13 commits into from
Jan 22, 2025
Merged

Add "new" labels to navigation #36939

merged 13 commits into from
Jan 22, 2025

Conversation

cinnamon-msft
Copy link
Collaborator

@cinnamon-msft cinnamon-msft commented Jan 17, 2025

Summary of the Pull Request

Added "New" in the nav to System Tools and ZoomIt now that we have a new utility!
image

image

image

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@htcfreek
Copy link
Collaborator

htcfreek commented Jan 17, 2025

Why not on dashboard too? Could be placed between label and switch button.

@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Jan 19, 2025

Questions

  • Nit: 5px is not really in line with general recommendation to use multiple of four.
  • Is decided how long a new something is marked as new?

@cinnamon-msft
Copy link
Collaborator Author

Questions

  • Nit: 5px is not really in line with general recommendation to use multiple of four.
  • Is decided how long a new something is marked as new?

Great callout on the px size, I'll change it to 4

Duration should only be for one release

Copy link
Collaborator

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

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

nit

HorizontalAlignment="Center"
VerticalAlignment="Center"
FontSize="{ThemeResource InfoBadgeValueFontSize}"
FontWeight="SemiBold" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
FontWeight="SemiBold" />

Copy link
Collaborator

Choose a reason for hiding this comment

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

No longer SemiBold, thank you for the feedback ;)

@htcfreek
Copy link
Collaborator

@cinnamon-msft
Can you align the badge on the dashboard card to the right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't edit this file but I think it started failing the XAML formatting check after I merged main in

<InfoBadge
Margin="4,0,0,0"
Style="{StaticResource NewInfoBadge}"
Visibility="{x:Bind IsNew, Converter={StaticResource BoolToVisibilityConverter}, Mode=OneWay}" />
Copy link
Collaborator

@Jay-o-Way Jay-o-Way Jan 21, 2025

Choose a reason for hiding this comment

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

BoolToVisibilityConverter is embedded, so may be omitted :) Mode=OneWay may be removed too, this value is not going to change. (default for x:Bind is OneTime)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should prefer as explicit as possible :) Makes intent easier to read when going through the code if one doesn't know the defaults.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@cinnamon-msft cinnamon-msft merged commit 318cb32 into main Jan 22, 2025
13 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants