-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 icon to profile title on settings UI #9694
Add icon to profile title on settings UI #9694
Comments
Hi, do you have any UI mockups (for dimensions, spacings etc) for the icon? |
## Summary of the Pull Request Adds the profile icons to the page header. I had to manually create the header, and manually bind it to the `Icon` and `Content` of each `NavViewItem`. It's important that each `NavViewItem`'s icon is set as an `IconSource`, so that we can bind to it. If it's just a plain old `FontIcon`, then we can't re-use it. Additionally, I removed the manual sizing of all font icons to font size 12. That would make font icons _tiny_ in the header. Now, they'll properly re-use the size of the `NavigationViewTitleHeaderContentControlTextStyle` in the nav view header. This involved also manually making the icons smaller on the `AddProfile` page and in the `CommandPalette`. As per usual, images are in Teams ## PR Checklist * [x] Closes #9694 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * Checked (bitmap|font) icons in tabs * (bitmap|font) icons in the flyout * (bitmap|font) icons in command palette * (bitmap|font) icons in the nav view * (bitmap|font) icons in the header * (bitmap|font) icons in the add profile page
…0124) This reverts commit a3a2a41. #10046 causes a crash on save. MainPage::UpdateSettings() is unable to update the navigation view's selected item due to an "incorrect parameter". This is particularly strange to see because #10046 only modifies the navigation view's header, not the menu items themselves. Reverting this change fixes that crash (verified). Reopens #9694
Hey can you assign this to me |
@HimanshuMahto Let us know if you need any help |
Since I'm new to this can you help me @zadjii-msft |
Sure thing! I'd start by looking at #10046. That was an earlier attempt to do this, which we ultimately reverted in #10124. We had to revert that due to some crash when you saved the settings, but I'm thinking that crash is probably no longer applicable? Now, the thing is, we've kinda re-written the settings UI since that original PR. It's still structurally pretty much the same, but there's been some minor changes to account for the Windows 11 style differences. You'll probably still want to put the icon in: terminal/src/cascadia/TerminalSettingsEditor/MainPage.xaml Lines 67 to 98 in 1745857
|
okay, I got it. hey, Can I get your contact so that I can ask you questions in person if you don't mind? @zadjii-msft |
To be totally honest - I'd prefer to keep discussion on GitHub. Reasons include (but aren't limited to):
|
okay, thanks for the advice. |
Hey, @zadjii-msft I'm new to this and don't know how to run and see the changes though I have installed powershell pro tools and powershell in the VS code and did try to run the code but it wasn't showing anything. Can you please help me. I know its a silly question but I don't know how to do it and want to learn want to contribute to this. |
Hey @zadjii-msft @DHowett @cinnamon-msft need a help |
@HimanshuMahto Did you try following these steps in the README? Once you've got the code checked out and the tools installed, the easiest way to test the code is to open up OpenConsole.sln, set the config to and hit the big green play button to start debugging. |
Got it @zadjii-msft. Thanks a lot i will start working on it. |
Hey, @zadjii-msft, I tried what you said but its still not working. |
Hii, seems like the last person was unsuccessful at this. Can you assign this to me? It will be my first issue. |
You can just go ahead and start working on things without necessarily being assigned. Usually we really only use the Assignees field if someone on the core team has to be the one to deal with an issue 😝 |
Hi @zadjii-msft, pretty much done with the implementation of this (i.e. just kinda copying your old code across). Did you ever manage to overcome the resolution scaling issues? The font icons look fine, but the powershell icon, when scaled up to a height of 28 looks quite horrible. |
@Kore-rep Are you still working on this? |
Sorry, I think I missed that earlier message 😅 I never really looked into the image scaling. It sounds like something I looked at briefly, where terminal/src/cascadia/UIHelpers/IconPathConverter.cpp Lines 335 to 336 in ae90d52
terminal/src/cascadia/UIHelpers/IconPathConverter.cpp Lines 158 to 160 in ae90d52
but I can't be sure. I'd reckon it's probably more valuable to put out a PR with this change as it is now. That'd force our hands to make sure we fix that :P |
Hi @aditya-sahu, I've opened a PR with my changes, feel free to make any changes you want to for that. I never resolved the image scaling issue, or did any particular refactoring to make things nicer, its a pretty barebones implementation |
Description of the new feature/enhancement
If would be cool if we displayed the icon of the profile at the top of the profile page on the settings UI.
Proposed technical implementation details (optional)
The text was updated successfully, but these errors were encountered: