Skip to content

Conversation

@0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Feb 23, 2023

Description

I'm redesigning storage object's properties pages.

Tasks to do

  • Update UI
  • Add PathIcons for all of NavViewItem icons
  • Format all codes that I changed
  • Add new translation resources

Validation

  • Built and ran the app
  • Tested the changes for accessibility

Screenshots

MS-Store-like vertical NavigationView
image

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

I left some comments on code style.
There are some other issues:

  • You can't scroll General tab (edit. also Compatibility)
    3

  • Resizing the window doesn't work as expected
    2

  • Shortcut glyph is wrong
    4

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 23, 2023

@ferrariofilippo Thank you for reviewing however those commits were created for showing a schreenshot to discuss it is ok that we leave as it is.

@0x5bfa 0x5bfa marked this pull request as draft February 23, 2023 10:39
@0x5bfa 0x5bfa changed the title Redesign Property pages Feature: Redesign Property pages Feb 23, 2023
@yaira2
Copy link
Member

yaira2 commented Feb 23, 2023

however this new style is quite looks bad and have some accessibility and user-experience problems.

Is it the outline icons? This will improve when I implemented the filled versions.

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 23, 2023

Yes. if so, you have to add all of that cuz Segoe and CustomGlyph cannot be coexisted. Do you do that in this PR?

<FontIcon
    x:Name="NavigationViewItemGlyphPrimeryFontIcon"
    Margin="2,0,0,0"
    HorizontalAlignment="Center"
    x:Load="{x:Bind IsSelected, Mode=OneWay}"
    FontSize="22"
    Foreground="{ThemeResource SystemAccentColor}"
    Glyph="{x:Bind GlyphPrimary, Mode=OneWay}" />

<FontIcon
    x:Name="NavigationViewItemGlyphSecondaryFontIcon"
    Margin="2,0,0,0"
    HorizontalAlignment="Center"
    x:Load="{x:Bind IsSelected, Converter={StaticResource BoolNegationConverter}, Mode=OneWay}"
    FontSize="22"
    Foreground="{ThemeResource TextFillColorSecondaryBrush}"
    Glyph="{x:Bind GlyphSecondary, Mode=OneWay}" />

@yaira2
Copy link
Member

yaira2 commented Feb 23, 2023

I'll take care of this, can you fix the scroll issues?

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 23, 2023

@yaira2 Scrolling problem was fixed.

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 23, 2023

  • You can't scroll General tab (edit. also Compatibility)
  • Resizing the window doesn't work as expected
  • Shortcut glyph is wrong(replaced Wrench icon)

@yaira2 yaira2 changed the title Feature: Redesign Property pages Feature: Vertical NavigationView in the properties window Feb 23, 2023
@0x5bfa 0x5bfa marked this pull request as ready for review February 24, 2023 16:55
@yaira2
Copy link
Member

yaira2 commented Feb 24, 2023

@onein528 does opening the properties window crash the app for you?

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 24, 2023

I'm gonna commit 'Some fix 12' including fix that crush.

@yaira2
Copy link
Member

yaira2 commented Feb 24, 2023

Thanks! It might be good practice to make the commit names a little more descriptive.

yaira2
yaira2 previously approved these changes Feb 24, 2023
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 24, 2023

Wait why removed that negative margin?

@yaira2
Copy link
Member

yaira2 commented Feb 24, 2023

It was misaligned, I'm trying to find a different solution.

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Feb 24, 2023
@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 24, 2023

Should/Can I remove unneeded controls from style in ResDictionary?

@yaira2
Copy link
Member

yaira2 commented Feb 24, 2023

It's a good idea to

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 24, 2023

Future task lists

  • Rename Pages
  • Rename ViewModels (especially, PropertiesTab)
  • Refactor codes in order not to use outdated terms like 'tab'
  • Redesign all properties pages to fit this new design

@yaira2 yaira2 merged commit 6946ddb into files-community:main Feb 24, 2023
@0x5bfa 0x5bfa deleted the 5bfa/redesign-propertypages branch February 24, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants