-
Notifications
You must be signed in to change notification settings - Fork 324
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
Adding "open new window" icon to "Create Dev Drive" button (#1334) #2062
Adding "open new window" icon to "Create Dev Drive" button (#1334) #2062
Conversation
…ucing Dev Home" page (microsoft#1334)
@microsoft-github-policy-service agree |
Hi @cinnamon-msft , does the image of the updated button look good (spacing, size, location, etc.)? If so, I'll publish this PR. Thanks! |
I would suggest making the icon a bit smaller to match the font size, but otherwise I think this looks great! |
e98ada9
to
37f2e51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary of the pull request
A request was made to clarify that clicking the "Create Dev Drive" button in the "Introducing Dev Home" page view takes you outside the app. @cinnamon-msft suggested that the same icon used in the "Machine Configuration" page should be added to convey this idea.
References and relevant issues
#1334
Detailed description of the pull request / Additional comments
In WhatsNewPage.xaml, some refactoring of the button element occurs. Child elements are added in order to accommodate both text and icon inside the button.
Additionally, I added a boolean property to the WhatsNewCard.cs called ShouldShowIcon which is used to toggle the Visibility property of the icon since currently the "Create Dev Drive" button is the only button we want the icon showing for.
In WhatsNewPage.xaml.cs, logic was added to set ShouldShowIcon to false except for the "Create Dev Drive" button.
Validation steps performed
Manually verified UI renders as expected
PR checklist