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

Feature: Integrate Advanced Security page into properties window #11930

Merged
merged 57 commits into from
Apr 20, 2023

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Apr 1, 2023

Feature: Integrate Advanced Security page into properties window

Motivation and Context

Planned by Files team

Details of changes

  • Integrate Advanced Security page into properties window
  • Update titlebar text styling
  • Update the height of each NavigationViewItem from 40px to 36px (32px is default)
  • Tweaked UI in all properties pages
  • Add backward navigation functionality in properties window
  • Use generalized Parameter class for every class in Properties

PR Checklist

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Go to properties window
    2. Navigate to Security tab
    3. Click 'Advanced permissions' button

Screenshots

Backward navigation button & new titlebar looks & Integrated SecurityAdvancedPage
image

@yaira2
Copy link
Member

yaira2 commented Apr 2, 2023

Is this ready for review?

@0x5bfa 0x5bfa changed the title Feature: Integrate Security Advanced page Feature: Integrate Security Advanced page & Add backward navigation in Properties window Apr 2, 2023
@0x5bfa 0x5bfa marked this pull request as draft April 2, 2023 16:29
@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 2, 2023

@yaira2 Yes done.

@0x5bfa 0x5bfa marked this pull request as ready for review April 2, 2023 19:07
yaira2
yaira2 previously approved these changes Apr 18, 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.

The UI changes look good to me.

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 think we should add the item name to the window name. If a user has multiple properties opened (not on the general page) it might be hard to tell which one belongs to which item

yaira2
yaira2 previously approved these changes Apr 19, 2023
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.

There are a couple things that might be changed. Apart from them, LGTM

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.

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Apr 20, 2023
@yaira2 yaira2 merged commit 0001dca into files-community:main Apr 20, 2023
@yaira2
Copy link
Member

yaira2 commented Apr 20, 2023

@0x5bfa thank you for all your hard work on this PR. This has been on the todo list for almost 2 years but it kept getting put off due to the size and other priorities so I'm happy it's finally taken care of.

@@ -289,7 +306,6 @@
BorderBrush="{ThemeResource CardStrokeColorDefaultBrush}"
BorderThickness="1"
ColumnSpacing="8"
CornerRadius="4"
Copy link
Member

Choose a reason for hiding this comment

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

@0x5bfa was this removed on purpose?

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