[PowerRename] Show UI info if item cannot be renamed #19934
Conversation
Highlight items that couldn't be renamed and add error message flyout
stefansjfw
requested review from
yuyoyuppe,
SeraphimaZykova,
niels9001,
sosssego and
jaimecbernardo
| <Border | ||
| x:Name="ErrorIcon" | ||
| Grid.Column="3" | ||
| Width="12" | ||
| Height="12" | ||
| CornerRadius="12" | ||
| Margin="4,0,8,0" | ||
| Background="{ThemeResource SystemFillColorCriticalBrush}" | ||
| Visibility="Collapsed"> | ||
| <TextBlock | ||
| HorizontalAlignment="Center" | ||
| VerticalAlignment="Center" | ||
| FontFamily="{ThemeResource SymbolThemeFontFamily}" | ||
| Foreground="White" | ||
| FontSize="8" | ||
| Text="" /> |
There was a problem hiding this comment.
Can we use glyph icons with the official brushes and symbols for the error icon in the muxc:InfoBar? This would fix the colors and make the x centered in the circle.
There was a problem hiding this comment.
@niels9001 E10A is not allowed. You should know this by now. Please remember our agreement.
And please never hard-code a font color.
There was a problem hiding this comment.
@stefansjfw
Did you see my review?
There was a problem hiding this comment.
yes, that's Q for @niels9001 :)
There was a problem hiding this comment.
@niels9001
Here is a list of all infobar theme resources: https://docs.microsoft.com/en-us/windows/winui/api/microsoft.ui.xaml.controls.infobar?view=winui-2.8#all-theme-resources
There was a problem hiding this comment.
@stefansjfw and @niels9001 - Using a <border> to achieve a colored background for an icon? Don't take it the wrong way, but I expect better from you guys (!) (try to find a compliment in there...)
Have a look at https://docs.microsoft.com/windows/apps/design/style/segoe-fluent-icons-font#layering-and-mirroring and let's use
<Grid>
Icon="InfoBarIconBackgroundGlyph" Foreground="InfoBarErrorSeverityIconBackground" // red color
Icon="InfoBarErrorIconGlyph Foreground="InfoBarErrorSeverityIconForeground"
</Grid>There was a problem hiding this comment.
@Jay-o-Way
I agree with and that's why I wrote it.
And btw. we can use the background of the error bar (InfoBarErrorSeverityBackgroundBrush) as background for the table row. Then we are in line with system colors.
There was a problem hiding this comment.
@niels9001
Here is a list of all infobar theme resources: https://docs.microsoft.com/en-us/windows/winui/api/microsoft.ui.xaml.controls.infobar?view=winui-2.8#all-theme-resources
The InfoBar brushes inherit other brushes... the one we use, so this is fine. See WinUI repo.
There was a problem hiding this comment.
For the brushes I am okay with what we have. But can we use the X-Glyph-Resource from info bar. The current implementation you did isn't centered on the red background and this looks not good. (See screenshots in PR description.)
|
Adding #8748 |
Address PR comment Co-authored-by: Jay <65828559+Jay-o-Way@users.noreply.github.com>
Address PR comment Co-authored-by: Jay <65828559+Jay-o-Way@users.noreply.github.com>
src/modules/powerrename/PowerRenameUILib/Strings/en-us/Resources.resw
Outdated
Show resolved
Hide resolved
…ys into stefan/powerrename_wip
stefansjfw
force-pushed
the
stefan/powerrename_wip
branch
from
8202233
to
660a647
Compare
| <Setter Target="HighlightBorder.Opacity" Value="0.1" /> | ||
| <Setter Target="HighlightBorder.BorderBrush" Value="{ThemeResource AccentTextFillColorPrimaryBrush}" /> | ||
| <Setter Target="HighlightBorder.Background" Value="{ThemeResource AccentTextFillColorPrimaryBrush}" /> | ||
| </VisualState.Setters> |
There was a problem hiding this comment.
Correct me if I'm mistaken, but I don't see a Icon visibility = none here. Isn't that needed to hide it when it's back to normal?
There was a problem hiding this comment.
No, the closing tag indicates that the Normal state falls back to the default states defined in the XAML - in this case, the icon is already collapsed.
EDIT: Oh, going from Error -> Highlight state? @stefansjfw Is that a valid scenario or will it go through the Normal state first at all times? If not we might want to add the collapsing of the Error icon in the Highlight state as @Jay-o-Way mentioned.
There was a problem hiding this comment.
Error -> Highlight state works as expeected
|
@Jay-o-Way @htcfreek Can we please keep the feedback relevant and condense? I admire the enthousiasm, but the threads sometimes get out of control quickly and it can become hard to review 😄. The reason why the error icon was "hardcoded" is because the WAS version we're using does not support the latest WinUI version yet - so we can't use the InfoBadge control. Once we move to WAS 1.2 we can remove this altogether and use InfoBadge. We have a similiar issue in Settings so we can do that all in a seperate PR. |
The resources I suggested aren't from InfoBadge control. The resources are from InfoBar control. For settings this is working on my PR. So if we have access to InfoBar control we should be able to use the suggested default resources. |
Is this related? Why overwrite default values? |
|
Can we at least change the <Border> as suggested? |
Let's polish it in separate PR |
Co-authored by: @niels9001
Summary of the Pull Request
Item that cannot be renamed will be highlighted in red and hovering over the error symbol will show a tooltip with info. Covered errors for now are:
Note: PowerRename uses API that is not Long Path aware for renaming. So it doesn't support renaming Long Path files and it stays that way for now, i.e. it'll show info that file path is longer than supported.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed